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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
