Review Request 115992: PCH support in kdev-clang

[Milian Wolff]

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



clangparsejob.cpp
<https://git.reviewboard.kde.org/r/115992/#comment35645>

    nitpick :) const QString&
    
    also, please add a comment to this function, what it expects the file to contain and that only the first .h is returned.



clangparsejob.cpp
<https://git.reviewboard.kde.org/r/115992/#comment35647>

    const



clangparsejob.cpp
<https://git.reviewboard.kde.org/r/115992/#comment35646>

    braces please everywhere - thanks!



clangparsejob.cpp
<https://git.reviewboard.kde.org/r/115992/#comment35654>

    make the defines const, no?



clangparsejob.cpp
<https://git.reviewboard.kde.org/r/115992/#comment35655>

    unused?



clangparsejob.cpp
<https://git.reviewboard.kde.org/r/115992/#comment35653>

    again, & and * next to type name



clangparsejob.cpp
<https://git.reviewboard.kde.org/r/115992/#comment35652>

    this is pretty ugly, could you move that to somewhere else? Maybe ClangIndex?



clangparsejob.cpp
<https://git.reviewboard.kde.org/r/115992/#comment35656>

    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



clangparsejob.cpp
<https://git.reviewboard.kde.org/r/115992/#comment35657>

    this with the below doesn't make any sense. use the urlparselock (see comment below) and only check the getExisting once



clangparsejob.cpp
<https://git.reviewboard.kde.org/r/115992/#comment35651>

    this can/should be done from within ClangParseJob::run
    
    it does not require to be run in the foreground thread after all.



clangparsejob.cpp
<https://git.reviewboard.kde.org/r/115992/#comment35650>

    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



duchain/parsesession.h
<https://git.reviewboard.kde.org/r/115992/#comment35649>

    Path&



duchain/parsesession.cpp
<https://git.reviewboard.kde.org/r/115992/#comment35603>

    nitpick Path&



duchain/parsesession.cpp
<https://git.reviewboard.kde.org/r/115992/#comment35648>

    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?)


- Milian Wolff


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/13bd44cf/attachment-0001.html>