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

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp:80
+        << Test;
+    EXPECT_THAT(collectKnownReferences(AST.getMacros()), AreMacroRefsFrom(T))
+        << Test;
----------------
hokein wrote:
> usaxena95 wrote:
> > 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.
> >   
> I see, having a complex `Matcher <std::vector<Matcher <std::vector<Range>>>>` would hurt code readability, and the error message is also not friendly when there are failed testcases.
> 
> How about extending it like below? To improve the error message, I think we should do a string comparison: the test annotation v.s the raw code with the actual macro references annotated (we'd need to write some code to compute this actual annotated code, you can see SemanticHighlightingTests.cpp for reference).  
> 
> ```
> #define $1[[a$1^bc]] 1
> #undef $1[[abc]]
> 
> #define $2[[a$2^bc]] 2
> #undef $2[[abc]]
> 
> #ifdef $Unknown[[abc]]
> #endif
> ```
I have tried to check against each group now. Also verifies the SymbolID. Failures will point the annotation for which the testcase failed.
Recreating the annotated code from the raw code would become more complex as it would involve maintaining a mapping from SymbolId->Annotation. Works quite nicely for SemanticHighlighting as we have the whole Token stream. Creating the replacements does not sound trivial to me. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70008