[PATCH] D38639: [clangd] #include statements support for Open definition

[Ilya Biryukov via Phabricator via cfe-commits]

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