Review Request 121757: Use target environment whenever available, and implement TU pinning (WIP)

Milian Wolff mail at milianw.de
Wed Jan 7 14:37:09 UTC 2015


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



clangparsejob.cpp
<https://git.reviewboard.kde.org/r/121757/#comment51079>

    why this change?



duchain/clangindex.h
<https://git.reviewboard.kde.org/r/121757/#comment51087>

    const&



duchain/clangindex.h
<https://git.reviewboard.kde.org/r/121757/#comment51088>

    const&



duchain/clangindex.h
<https://git.reviewboard.kde.org/r/121757/#comment51089>

    const&



duchain/clangindex.cpp
<https://git.reviewboard.kde.org/r/121757/#comment51091>

    const&



duchain/clangindex.cpp
<https://git.reviewboard.kde.org/r/121757/#comment51090>

    this just means that the string is empty, which should never happen, no? or what do you want to check here? when does this occur?



duchain/clangindex.cpp
<https://git.reviewboard.kde.org/r/121757/#comment51095>

    maybe you want to check whether the file exists? Via QFile::exists ?



duchain/clangindex.cpp
<https://git.reviewboard.kde.org/r/121757/#comment51093>

    classic race condition of double-checked locking. after the unlock, another thread might add it. are you sure this is correct here? why not just always use a plain QMutex instead of the read/write lock? do you really expect high contention (I don't). And a non-recursive QMutex will be faster than the QReadLocker as well afaik.



duchain/clangindex.cpp
<https://git.reviewboard.kde.org/r/121757/#comment51094>

    join with previous line, or remove else alltogether



duchain/clangindex.cpp
<https://git.reviewboard.kde.org/r/121757/#comment51085>

    const&



duchain/clangindex.cpp
<https://git.reviewboard.kde.org/r/121757/#comment51086>

    const&



duchain/clangindex.cpp
<https://git.reviewboard.kde.org/r/121757/#comment51097>

    I'd restructure this, esp. if this could be called multiple times. currently, your "setTUImports" doesn't set, but addsTUImports - intended? If so, rename. Otherwise:
    
        auto tuImports = toSet(imports) << tu;
        Lock;
        m_urlsInTu[tu] = tuImports;



duchain/clangparsingenvironment.h
<https://git.reviewboard.kde.org/r/121757/#comment51083>

    const&
    
    and maybe setTuUrl? below, it's also tuUrl after all.



duchain/clangparsingenvironment.cpp
<https://git.reviewboard.kde.org/r/121757/#comment51082>

    const&



duchain/parsesession.h
<https://git.reviewboard.kde.org/r/121757/#comment51080>

    why this change? I find it odd to have code like "session.sessionUrl" instead of just "session.url". same for file.



duchain/parsesession.h
<https://git.reviewboard.kde.org/r/121757/#comment51081>

    const&



duchain/tuduchain.cpp
<https://git.reviewboard.kde.org/r/121757/#comment51099>

    unrelated change? +1 though, feel free to commit as-is.



tests/test_duchain.cpp
<https://git.reviewboard.kde.org/r/121757/#comment51100>

    this is ugly. why is this required now?


- Milian Wolff


On Jan. 5, 2015, 3:13 p.m., Olivier Jean de Gaalon wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121757/
> -----------------------------------------------------------
> 
> (Updated Jan. 5, 2015, 3:13 p.m.)
> 
> 
> Review request for KDevelop, Kevin Funk and Milian Wolff.
> 
> 
> Repository: kdev-clang
> 
> 
> Description
> -------
> 
> Note: This requires IBuildSystemManager::hasIncludesOrDefines, which should probably be renamed to isTranslationUnit()
> 
> The idea here is to use the AST of the buildsystem's translation units wherever possible, since that's the only file that's guaranteed to have correct defines and includes (and furthermore can handle strange cases such as '#include inside namespace' and picky include/define ordering).
> This proposal essentially allows TU environments to override non-TU environments and then get "pinned" so that the non-TU will continue to use the TU AST to build its duchain.
> This could eventually be exposed in the UI, allowing the user to pick different "views" of headers (overriding the pinned TU).
> The pinning data needs to be fleshed out to handle expiry (header no longer included) and be serialized with an ItemRepository.
> 
> I'm looking for feedback on this idea... where does it break? Any better ideas?
> 
> This more or less works, we can flesh it out more if we want to take this route.
> 
> 
> Diffs
> -----
> 
>   tests/clang-parser.cpp 3f87d05 
>   duchain/tuduchain.h dfcc439 
>   duchain/tuduchain.cpp 33a0570 
>   duchain/clangindex.cpp 02c93cf 
>   duchain/clangparsingenvironment.h 7bb8111 
>   duchain/clangparsingenvironment.cpp 1decc14 
>   duchain/clangparsingenvironmentfile.h 953ee94 
>   duchain/clangparsingenvironmentfile.cpp b3d0563 
>   duchain/clangpch.cpp 1ce3457 
>   duchain/debugvisitor.cpp be8debf 
>   duchain/parsesession.h b688fb1 
>   duchain/parsesession.cpp f41768a 
>   tests/test_duchain.cpp 7db9fea 
>   tests/test_problems.cpp 09018d1 
>   clangparsejob.h 1cc8282 
>   clangparsejob.cpp 297b836 
>   codecompletion/completionhelper.cpp f5ed2b9 
>   codecompletion/context.cpp 33c3391 
>   codegen/clangsignatureassistant.cpp 9ea1f5b 
>   duchain/clanghelpers.h 4216454 
>   duchain/clanghelpers.cpp 5f29a2a 
>   duchain/clangindex.h 991c7ca 
> 
> Diff: https://git.reviewboard.kde.org/r/121757/diff/
> 
> 
> Testing
> -------
> 
> Tests fail in the same manner as before
> 
> 
> File Attachments
> ----------------
> 
> kdevelop patch
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/01/02/5c0314bf-f241-4ad0-93f6-9a0a48a79135__kdevelop-hasincordef.diff
> platform patch
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/01/02/edc11881-06c6-4570-9afd-824a99687116__kdevplatform-hasincordef.diff
> 
> 
> Thanks,
> 
> Olivier Jean de Gaalon
> 
>

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


More information about the KDevelop-devel mailing list