Review Request 115992: PCH support in kdev-clang

[Olivier Jean de Gaalon]

> On March 1, 2014, 5:04 p.m., Milian Wolff wrote:
> > duchain/clanghelpers.h, line 48
> > <https://git.reviewboard.kde.org/r/115992/diff/2/?file=251095#file251095line48>
> >
> >     why are these two not exported?

They aren't used outside of the library ..?


> On March 1, 2014, 5:04 p.m., Milian Wolff wrote:
> > duchain/clangpch.cpp, line 48
> > <https://git.reviewboard.kde.org/r/115992/diff/2/?file=251098#file251098line48>
> >
> >     what's this about? why is this static? constructing an empty path is cheap so you could leave it non-static?

Documentary purposes (here is where you handle chained pch), but sure, meh.


> On March 1, 2014, 5:04 p.m., Milian Wolff wrote:
> > duchain/clangtypes.cpp, line 58
> > <https://git.reviewboard.kde.org/r/115992/diff/2/?file=251100#file251100line58>
> >
> >     the QFileInfo check could be done outside the QReadLocker, no? Actually even before the UrlParseLock? or do you think it might get deleted inbetween?
> >     
> >     Generally, this can be simplified by doing this (which is also non-detaching btw):
> >     
> >     if (QFile::exists(pchInclude.toLocalFile() + pchExt)) {
> >       QReadLocker lock(&m_pchLock);
> >       auto pch = m_pch.value(pchInclude);
> >       if (pch) {
> >         return pch;
> >       }
> >     }

If the QFileInfo check is done before the UrlParseLock, multiple ClangParseJobs using the same TU may commit to creating the PCH after simultaneously discovering it doesn't exist. It shouldn't cause problems, as each creation will still be serialized, but it's not the correct behavior.


> On March 1, 2014, 5:04 p.m., Milian Wolff wrote:
> > clangparsejob.cpp, line 128
> > <https://git.reviewboard.kde.org/r/115992/diff/2/?file=251093#file251093line128>
> >
> >     FYI: there's also QLatin1String

Sure, but then you still have to create a QString each time you use it, at least in principle.


- Olivier Jean de


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115992/#review51442
-----------------------------------------------------------


On Feb. 28, 2014, 7:12 p.m., Olivier Jean de Gaalon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115992/
> -----------------------------------------------------------
> 
> (Updated Feb. 28, 2014, 7:12 p.m.)
> 
> 
> Review request for KDevelop and Milian Wolff.
> 
> 
> Repository: kdev-clang
> 
> 
> Description
> -------
> 
> This is a little dirty, but I probably won't be able to work on it any more this week, so at least I can throw it up for feedback. Give special attention to parselock, because I didn't take the time to see how it's supposed to be used.
> 
> 
> Diffs
> -----
> 
>   clangparsejob.h b12e9df 
>   clangparsejob.cpp 86588ce 
>   duchain/CMakeLists.txt 7ab2e8a 
>   duchain/clanghelpers.h PRE-CREATION 
>   duchain/clanghelpers.cpp PRE-CREATION 
>   duchain/clangpch.h PRE-CREATION 
>   duchain/clangpch.cpp PRE-CREATION 
>   duchain/clangtypes.h b4baba7 
>   duchain/clangtypes.cpp f8eb8b8 
>   duchain/includedfilecontexts.h 4bdd6ca 
>   duchain/includedfilecontexts.cpp 90d800c 
>   duchain/parsesession.h e557884 
>   duchain/parsesession.cpp 265e099 
>   duchain/tuduchain.h e0e8d44 
> 
> Diff: https://git.reviewboard.kde.org/r/115992/diff/
> 
> 
> Testing
> -------
> 
> It works, including with epic-size projects. It still doesn't resume properly for some reason (clang will complain about invalid PCH, create an invalid tu, and the pch contexts will be nuked). I haven't had time to look into that, just have to remove the pch and rebuild for now.
> 
> 
> Thanks,
> 
> Olivier Jean de Gaalon
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20140301/f8714791/attachment-0001.html>