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

[Milian Wolff]

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


Hey Olivier, nice to see you working on kdev-clang again :] You are missing the sources for `tumapping.*`. But from what I understand, you'll have a mutex-guarded QHash or such in there that maps any IndexedString to another one that represents the TU. That would be the last .cpp that included a given header, correct? Meaning, whenever a .cpp is parsed, all the headers it includes (i.e. most of STL and Qt) are continously re-pinned in the mapping? Or would it only be pinned once?

What would happen if a header with insufficient defines/include paths is parsed, it would again repin the headers it includes, right? So esp. for local includes that could be bad:

foo.h (includes bar.h)
bar.h (includes some Qt header or such)
bar.cpp (includes bar.h)

-> if foo.h is parsed after bar.h and bar.cpp and no include paths are found for foo.h, then bar.h would be found (local lookup), but the Qt headers are not found. Then bar.h is "updated" from this problematic include set and pinned to foo.h? Or is there some safety in the code that I missed which would prevent that from happening, and bar.h would still be pinned to bar.cpp?

Also, I wonder why you pass the TU url to the Clang API, as mentioned below - please explain this a bit more to me.

Unrelated to the issues I point out, I agree that this approach is far more sophisticated than my simple approach with the buddy system. I also agree that we could potentially merge the two, such that the buddy is still requested for include paths and defines when we did not find a TU mapping.


duchain/clangparsingenvironmentfile.cpp
<https://git.reviewboard.kde.org/r/121757/#comment50806>

    rename this to quality



duchain/parsesession.cpp
<https://git.reviewboard.kde.org/r/121757/#comment50807>

    mod? I don't like the prefix name. maybe rather rename url to tuUrl here and keep the old url + contents?



duchain/parsesession.cpp
<https://git.reviewboard.kde.org/r/121757/#comment50808>

    hmmm I wonder what effect that has on clang. why do you pass the TU url here? wouldn't that break the caching in clang, i.e. when a file is reparsed? can you comment some more on your design decision here?



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

    there is no test for ::Project, is there?


- Milian Wolff


On Jan. 2, 2015, 1:51 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. 2, 2015, 1:51 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
> -----
> 
>   clangparsejob.h 1cc8282 
>   clangparsejob.cpp 297b836 
>   duchain/CMakeLists.txt 72464a5 
>   duchain/clanghelpers.cpp 5f29a2a 
>   duchain/clangparsingenvironment.h 7bb8111 
>   duchain/clangparsingenvironment.cpp 1decc14 
>   duchain/clangparsingenvironmentfile.h 953ee94 
>   duchain/clangparsingenvironmentfile.cpp b3d0563 
>   duchain/clangpch.cpp 1ce3457 
>   duchain/parsesession.h b688fb1 
>   duchain/parsesession.cpp f41768a 
>   tests/clang-parser.cpp 3f87d05 
>   tests/test_duchain.cpp 7db9fea 
>   tests/test_problems.cpp 09018d1 
> 
> 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/20150103/a3aa9259/attachment-0001.html>