Review Request 115992: PCH support in kdev-clang

Tue Feb 25 09:02:43 UTC 2014

> On Feb. 25, 2014, 8:48 a.m., Milian Wolff wrote:
> > clangparsejob.cpp, line 189
> > <https://git.reviewboard.kde.org/r/115992/diff/1/?file=245796#file245796line189>
> >
> >     braces please everywhere - thanks!

Hey you let this slide when the last guy wrote it!

> On Feb. 25, 2014, 8:48 a.m., Milian Wolff wrote:
> > clangparsejob.cpp, line 265
> > <https://git.reviewboard.kde.org/r/115992/diff/1/?file=245796#file245796line265>
> >
> >     unused?

Yikes, it should be used. It should be passed to ParseSession instead of pchInclude.

> On Feb. 25, 2014, 8:48 a.m., Milian Wolff wrote:
> > clangparsejob.cpp, line 320
> > <https://git.reviewboard.kde.org/r/115992/diff/1/?file=245796#file245796line320>
> >
> >     this is pretty ugly, could you move that to somewhere else? Maybe ClangIndex?

Yes, I don't intend for this to be production. When the correct locking scheme is worked out this can be done nicely. ClangIndex sounds odd to me though.

> On Feb. 25, 2014, 8:48 a.m., Milian Wolff wrote:
> > clangparsejob.cpp, line 324
> > <https://git.reviewboard.kde.org/r/115992/diff/1/?file=245796#file245796line324>
> >
> >     personally I'm not a big fan of helper lambdas like that - if you'd name that (i.e. put it into a static free function) you'd get quick open navigation etc. pp....
> >     
> >     looking at the use below, you only need this code once though

I need/use it twice. First it read-locks and tries to getExisting PCH, and if that fails it writelocks in preparation for creating the PCH. But after aquiring the writelock it needs to check again in case another PCH has beat it to the creation of PCH.
Of course, that's the theory, in practice ParseLock apparently doesn't do anything of the sort. Probably I'll just go with a custom PCH manager instead of this function that manages its own r/w locks.

> On Feb. 25, 2014, 8:48 a.m., Milian Wolff wrote:
> > clangparsejob.cpp, line 334
> > <https://git.reviewboard.kde.org/r/115992/diff/1/?file=245796#file245796line334>
> >
> >     this with the below doesn't make any sense. use the urlparselock (see comment below) and only check the getExisting once

It doesn't make sense if you actually know what parselock does. It makes a modicum of sense if it did what I assumed it did.

> On Feb. 25, 2014, 8:48 a.m., Milian Wolff wrote:
> > clangparsejob.cpp, line 363
> > <https://git.reviewboard.kde.org/r/115992/diff/1/?file=245796#file245796line363>
> >
> >     this can/should be done from within ClangParseJob::run
> >     
> >     it does not require to be run in the foreground thread after all.

Why? This doesn't actually create anything, it's identical to getUserDefinedIncludePaths.

> On Feb. 25, 2014, 8:48 a.m., Milian Wolff wrote:
> > clangparsejob.cpp, line 379
> > <https://git.reviewboard.kde.org/r/115992/diff/1/?file=245796#file245796line379>
> >
> >     I wouldn't pass around the parselock, just switch these lines and you should be set. Note that the parselock has a very bad name - it's just to verify that kdev is not shutting down while a parsejob is running.
> >     
> >     what you should do though is getting an UrlParseLock for the pch file found in m_pchInclude before parsing the PCH. Otherwise, you'll potentially parse the same pch multiple times.
> >     
> >     what you might want to check is whether abort was requested in between, like after getting the urlparselock and before parsing the pch

My parselock assumptions were all BS.

> On Feb. 25, 2014, 8:48 a.m., Milian Wolff wrote:
> > duchain/parsesession.cpp, line 190
> > <https://git.reviewboard.kde.org/r/115992/diff/1/?file=245798#file245798line190>
> >
> >     add a comment regarding the 0/1 here please to clarify why this is required (I guess since pch stuff should never use anything from the editor?)

And we don't have the pch file contents, so that's an empty QByteArray that we definitely don't want to use.

- Olivier Jean de

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

On Feb. 24, 2014, 9:56 a.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. 24, 2014, 9:56 a.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/parsesession.h e557884 
>   duchain/parsesession.cpp 265e099 
> 
> 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/20140225/e849e73f/attachment.html>