ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
Looking forward to getting this change! I miss this as well.
Please take a look at my comments, though. I think we might want to use a
different API to implement this.
================
Comment at: clangd/ClangdServer.cpp:292
+ std::shared_ptr<const PreambleData> StalePreamble =
+ Resources->getPossiblyStalePreamble();
+ if (StalePreamble)
----------------
We can't use `getPossiblyStalePreamble()`, we want latest state of the
`Preamble`. Use `getPreamble` instead.
================
Comment at: clangd/ClangdServer.cpp:294
+ if (StalePreamble)
+ IncludeMap = StalePreamble->IncludeMap;
+ Resources->getAST().get()->runUnderLock(
----------------
We don't need to copy the whole map here, better add a method to `PreambleData`
that does actual lookups for the source range.
================
Comment at: clangd/ClangdUnit.cpp:103
+ void AfterExecute(CompilerInstance &CI) override {
+ const SourceManager &SM = CI.getSourceManager();
----------------
There's a much better public API to get all includes that were encountered by
the `Preprocessor`: we need to override `PPCallbacks ::InclusionDirective`.
`PrecompiledPreamble` does not currently expose this callbacks, but could you
add to `PreambleCallbacks` in a separate commit?
================
Comment at: clangd/ClangdUnit.cpp:151
std::vector<serialization::DeclID> TopLevelDeclIDs;
+ std::map<SourceLocation, std::string> IncludeMap;
};
----------------
Please use our descriptive `Path` typedef.
================
Comment at: clangd/ClangdUnit.cpp:834
+
+ for (auto it = IncludeMap.begin(); it != IncludeMap.end(); ++it) {
+ SourceLocation L = it->first;
----------------
Can we mix in the results from includes outside `DeclarationLocationsFinder`?
================
Comment at: clangd/ClangdUnit.h:136
std::vector<DiagWithFixIts> Diags;
+ std::map<SourceLocation, std::string> IncludeMap;
};
----------------
`std::unordered_map` is a better fit here, why not use it?
================
Comment at: clangd/ClangdUnit.h:136
std::vector<DiagWithFixIts> Diags;
+ std::map<SourceLocation, std::string> IncludeMap;
};
----------------
ilya-biryukov wrote:
> `std::unordered_map` is a better fit here, why not use it?
Using `SourceLocation` after `SourceManager` owning them dies is somewhat hacky.
Please store clangd's `Range`s instead. You can get the ranges via
`PPCallbacks` method `InclusionDirective`, it has a parameter `CharSourceRange
FilenameRange`, exactly what we need here.
================
Comment at: unittests/clangd/ClangdTests.cpp:991
+ #include "foo.h"
+ #include "invalid.h"
+ int b = a;
----------------
Some includes may come outside of preamble, I would expect current
implementation to fail for this case:
```
#include <vector> // <-- works here
// preamble ends here
int main() {
#include "my_include.h" // <-- does not work here
}
```
https://reviews.llvm.org/D38639
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
