jvikstrom added inline comments.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:170
+ return;
+ Loc = SM.getSpellingLoc(Loc);
+ }
----------------
hokein wrote:
> hokein wrote:
> > jvikstrom wrote:
> > > hokein wrote:
> > > > The Loc here maybe not in the main file, considering the case
> > > >
> > > > ```
> > > > // in .h
> > > > #define DEFINE(X) in X;
> > > > #define DEFINE_Y DEFINE(Y)
> > > >
> > > > // in .cc
> > > >
> > > > DEFINE_Y
> > > > ```
> > > The spelling loc is still going to be in `DEFINE_Y` I think. And we only
> > > highlight arguments in macros. (Added a testcase though)
> > ok, I don't have an exact case for macros now, but my gut feeling tells me
> > we will encounter cases where the Loc is not in main file.
> >
> > here is a test case for declarations, we will visit the `foo()` decl in
> > `test.h` as well. This could be addressed in a separate patch.
> >
> > ```
> > // test.h
> > void foo();
> >
> > // test.cc
> > #include "test.h"
> > void foo() {}
> > ```
> >
> ok, here is the case, the spelling loc is not in main file, it is in
> `<scratch space>`
>
> ```
> // test.h
> #DEFINE(X) class X {};
> #DEFINE_Y(Y) DEFINE(x##Y)
>
> // test.cc
> DEFINE_Y(a);
> ```
You are correct. The other comment was actually correct as well, I just put the
arguments for the header code in the place where the source code should be in.
Added a check in addToken that the Loc we are trying to add must be in the main
file.
================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:195
+ #define DEF_CLASS(T) class T {};
+ DEF_MULTIPLE(XYZ);
+ DEF_MULTIPLE(XYZW);
----------------
ilya-biryukov wrote:
> Could you add a comment explaining that we choose to not highlight the
> conflicting tokens?
There is a comment in SemanticHighlighting.cpp at line 43. Want me to add a
comment in the test case as well?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64741/new/
https://reviews.llvm.org/D64741
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
