We were a) passing in a wrong file size, and b) a different set of unsaved
files on building the translation unit as compared to invoking completion.
This resulted in the precompiled preamble being rebuilt every single time.
On my test file, time spent in clang_codeCompleteAt goes down from ~700ms
to ~12ms with this change after the project is fully parsed.
It still doesn't work in header files at all.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
LGTM.
Do you think we should attempt to backport to 5.1 branch? We'd need to #ifdef the CXTranslationUnit_CreatePreambleOnFirstParse though since we still support older libclang there
| languages/clang/duchain/parsesession.cpp | ||
|---|---|---|
| 186 | Looks good to me. CXXChainPCH seems deprecated anyway: CXTranslationUnit_CXXChainedPCH DEPRECATED: Enabled chained precompiled preambles in C++. Note: this is a temporary option that is available only while we are testing C++ precompiled preamble support. It is deprecated. | |
| 198 | Good find. That needs libclang 3.9 though, if I understand correctly. I'm fine with bumping the dependency in master though. | |
fix it then push it - thanks!
| languages/clang/codecompletion/context.cpp | ||
|---|---|---|
| 784 | ugh - I hoped to not use the foreground lock in kdev code in order to remove it eventually. But fair enough for now, leave it as is. | |
| languages/clang/duchain/parsesession.cpp | ||
| 186 | any idea why we added this in the first place? can you also add something to your commit message saying why you remove this now? it seems somewhat unrelated to the rest (?) | |
| 198 | yeah, better add the ifdef? | |
| languages/clang/util/clangutils.cpp | ||
| 70 | remove spaces inside parens | |
| 73 | split onto two lines, i.e. after the first comma | |
| languages/clang/util/clangutils.h | ||
| 73 | please add apidox and note that this must be done from the foreground / have the foreground locked | |
Thanks for the reviews, I'll submit a fixed version tomorrow. I would not backport this tbh, but instead release 5.2 as soon as possible; I feel like the implications are relatively far-reaching and that shouldn't be in a point release. People who want to try the fix can use our 5.2 AppImage (with up-to-date clang).
| languages/clang/duchain/parsesession.cpp | ||
|---|---|---|
| 186 | It was supposed to tackle a similar problem but is now deprecated, according to documentation. I assume it does nothing right now anyways, so I removed it while touching that code. | |
| 198 | Yes, I'll ifdef that | |