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

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 marked 7 inline comments as done.
usaxena95 added inline comments.


================
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)) {
----------------
hokein wrote:
> 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.
Removed looking up xref.


================
Comment at: clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp:80
+        << Test;
+    EXPECT_THAT(collectKnownReferences(AST.getMacros()), AreMacroRefsFrom(T))
+        << Test;
----------------
hokein wrote:
> 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]]
> ```
Since the output of CollectMacros is a mapping from SymbolID -> vector, I wanted to verify that we form correct groups of ranges (grouped by SymbolID).
So a single macro per test case wouldn't be able to test this.

> 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.

We actually group the ranges by their annotation and form `Matcher <std::vector<Matcher <std::vector<Range>>>>`. So it checks for the correct grouping too.
  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70008