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/#review51442
-----------------------------------------------------------


looks very good - thanks Olivier! Mostly style nitpicks from my side. The only issue I found is with the locking in getPCH.


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

    FYI: there's also QLatin1String



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

    I prefer doxygen style comments for functions, i.e.
    
    /**
     * File should ...
     * @return the first path in the file
     */



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

    I know I let this in before, but I don't like the "get" prefix at all :-/ could you remove it everywhere please?



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

    Ternary operators for this is fine imo:
    
    return paths.isEmpty() ? {} : paths.first();



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

    this should be moved after the parselock, such that shutdown order is preserved properly



duchain/clanghelpers.h
<https://git.reviewboard.kde.org/r/115992/#comment36222>

    -get and api documentation for the stuff would be neato :)



duchain/clanghelpers.h
<https://git.reviewboard.kde.org/r/115992/#comment36221>

    why are these two not exported?



duchain/clangpch.cpp
<https://git.reviewboard.kde.org/r/115992/#comment36223>

    I didn't contribute anything here, did I :)



duchain/clangpch.cpp
<https://git.reviewboard.kde.org/r/115992/#comment36224>

    what's this about? why is this static? constructing an empty path is cheap so you could leave it non-static?



duchain/clangtypes.h
<https://git.reviewboard.kde.org/r/115992/#comment36228>

    please document this, esp. that it's threadsafe
    
    also, again, I dislike the get prefix :P



duchain/clangtypes.cpp
<https://git.reviewboard.kde.org/r/115992/#comment36225>

    use constFind



duchain/clangtypes.cpp
<https://git.reviewboard.kde.org/r/115992/#comment36226>

    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;
      }
    }



duchain/clangtypes.cpp
<https://git.reviewboard.kde.org/r/115992/#comment36227>

    create the pch without holding the write lock, and only insert it into m_pch with the writelock - this should decrease contention here.



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

    braces please :)


- Milian Wolff


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