[PATCH] D70008: [clangd] Store xref for Macros in ParsedAST.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/CollectMacros.h:29
   std::vector<Range> Ranges;
+  llvm::DenseMap<SymbolID, std::set<Location>> MacroRefs;
 };
----------------
usaxena95 wrote:
> hokein wrote:
> > I think the `Ranges` and `MacrosRefs` have a lot of duplications, it is wasteful to store a same range twice. We don't need the MainFilePath, the callers should know the corresponding file path for the AST. Something like `llvm::DenseMap<SymbolID, std::vector<Range>> Refs;` should be enough.
> > 
> > symbol id for macro generation is required the definition location, but only defined macros have it, we may need an extra `vector` to store ranges for undefined macros.
> > 
> > ```
> > #ifndef abc // there is no macro info* for abc, so getSymbolID will fail, but we still want it, e.g. in semantic highlighting.
> > #endif
> > ``` 
> > 
> > There may be more tricky cases, it would be nice to have in the test.
> > 
> > ```
> > #define abc 1
> > #undef abc
> > 
> > #define abc 2
> > #undef abc
> > ```
> - using `llvm::DenseMap<SymbolID, std::set<Range>> Refs;` as de-duplication is easier.
> - Iterating on all the ranges is a pain. Let me know if you want to provide a different method to return all the ranges (sad part: lot of copying).
> - I had multiple definitions of a macro in the test case, but I have shifted to its separate test case.
> as de-duplication is easier.

will we encounter duplications? My theory is that we should not. I think it is fine to use vector. And you also use vector for the `UnknownMacros` below.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:314
   });
   // Add highlightings for macro expansions.
+  for (const auto &SIDToRefs : AST.getMacros().MacroRefs) {
----------------
nit: the document was stale, expansions => references.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:898
+  // Handle macros.
+  if (auto Macro = locateMacroAt(Loc, AST.getPreprocessor())) {
+    if (auto MacroSID = getSymbolID(Macro->Name, Macro->Info, SM)) {
----------------
usaxena95 wrote:
> hokein wrote:
> > this is not completed, it only shows the macro refs in main file, we still need to query the index to get the rest (but our index doesn't collect macro at the moment). I'd prefer not doing this in this patch until our index is ready.
> > 
> > For testing, I think we can create a new test file for `CollectMainFileMacros`.
> Yup. This patch is just for the main file. The index and the lookup will be modified in the next patch. Wanted to keep this small.
> 
this would enable the xref for macros, but the feature is not ready yet. I think we'd better add this code when everything is ready.


================
Comment at: clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp:56
+      R"cpp(
+        #ifdef $Unknown[[UNDEFINED]]
+        #endif
----------------
could you add test case where there is an undefined macro reference before the macro definition, like

```
#ifndef abc
#define abc
#endif
#endif
```

I think the first `abc` is unknown, the second abc is defined?



================
Comment at: clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp:80
+        << Test;
+    EXPECT_THAT(collectKnownReferences(AST.getMacros()), AreMacroRefsFrom(T))
+        << Test;
----------------
I might be missing something, I didn't get the motivation of using numbers in the annotation, the code seems just collecting all annotated ranges regardless the value of the number and compare to the actual ranges. it doesn't verify that e.g. in your 2rd cases, the two macros `abc` are different symbols.

How about just verifying a single defined macro for each case? e.g. the below case, we try to get the symbol id (call `locatedMacroAt`) from the T.point(), retrieve the responding ranges from the MainFileMacros, compare to the annotated ranges.

```
#define [[F^oo]] 1
int abc = [[Foo]];
#undef [[Foo]]
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70008/new/

https://reviews.llvm.org/D70008