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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks, looks mostly good, a few more nits.



================
Comment at: clang-tools-extra/clangd/CollectMacros.h:91-92
       Out.Names.insert(MacroNameTok.getIdentifierInfo()->getName());
-      Out.Ranges.push_back(*Range);
+      if (auto SID =
+               getSymbolID(MacroNameTok.getIdentifierInfo()->getName(), MI, SM))
+        Out.MacroRefs[*SID].push_back(*Range);
----------------
nit: abstract `MacroNameTok.getIdentifierInfo()->getName()` to a variable? we also used this on line 90.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:898
       getBeginningOfIdentifier(Pos, SM, AST.getASTContext().getLangOpts()));
+
   // TODO: should we handle macros, too?
----------------
nit: remove this change.


================
Comment at: clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp:80
+        << Test;
+    EXPECT_THAT(collectKnownReferences(AST.getMacros()), AreMacroRefsFrom(T))
+        << Test;
----------------
usaxena95 wrote:
> 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. 
Thanks, this looks better now.


================
Comment at: clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp:1
+#include "Annotations.h"
+#include "CollectMacros.h"
----------------
nit: add missing license.


================
Comment at: clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp:28
+      )cpp",
+      // FIXME: Locating macro in duplicate definitions doesn't work. Enable
+      // this once LocateMacro is fixed.
----------------
This is interesting, by "doesn't work", you mean the function could not locate the correct definitions for (the first & second `abc`)?


================
Comment at: clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp:49
+        #endif
+      )cpp",
+      R"cpp(
----------------
could you add one more test case where there is a macro reference in another macro argument, like:

```
#define M(X) X;
#define $1[[abc]] 123
int s = M($1[[abc]]);
```


================
Comment at: clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp:74
+    for (int I = 1;; I++) {
+      const auto ExpectedRefs = T.ranges(llvm::to_string(I));
+      if (ExpectedRefs.empty())
----------------
nit: const auto &


================
Comment at: clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp:98
+} // namespace clang
\ No newline at end of file

----------------
nit: we need newline at EOF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70008