[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

This revision was automatically updated to reflect the committed changes. jvikstrom marked 2 inline comments as done. Closed by commit rL369275: [clangd] Added highlighting for tokens that are macro arguments. (authored by jvikstrom, committed by ). Herald added a project: LLVM. Herald added a

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

ilya-biryukov accepted this revision. ilya-biryukov marked an inline comment as done. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM from my side (and a few NIT, but up to you whether to apply them) Comment at:

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

jvikstrom added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:54 + if (Conflicting.size() > 1) { +Tokens.erase(Tokens.begin() + I, + Tokens.begin() + I + Conflicting.size()); ilya-biryukov wrote:

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

jvikstrom updated this revision to Diff 215907. jvikstrom marked 2 inline comments as done. jvikstrom added a comment. Rewrote conflicting token removal code. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64741/new/ https://reviews.llvm.org/D64741

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:332 + #define DEF_CLASS(T) class T {}; + DEF_MULTIPLE(XYZ); + DEF_MULTIPLE(XYZW); Unrelated to the change: do we plan to highlight

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

ilya-biryukov added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:54 + if (Conflicting.size() > 1) { +Tokens.erase(Tokens.begin() + I, + Tokens.begin() + I + Conflicting.size()); This is

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

jvikstrom added a comment. @ilya-biryukov @hokein ping 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

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

jvikstrom added a comment. @ilya-biryukov @hokein pinging about this cl (I had completely forgotten about it somehow. Just updated it to the new master though) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64741/new/

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

jvikstrom updated this revision to Diff 214620. jvikstrom added a comment. Moved comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64741/new/ https://reviews.llvm.org/D64741 Files: clang-tools-extra/clangd/SemanticHighlighting.cpp

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

jvikstrom updated this revision to Diff 214619. jvikstrom added a comment. Rebased to master. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64741/new/ https://reviews.llvm.org/D64741 Files: clang-tools-extra/clangd/SemanticHighlighting.cpp

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

jvikstrom updated this revision to Diff 210530. jvikstrom added a comment. Added comment saying conflicting tokens are not highlighted in the test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64741/new/ https://reviews.llvm.org/D64741 Files:

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

jvikstrom marked an inline comment as done. jvikstrom added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:46 +// removed. +for (unsigned I = 0; I < Tokens.size(); ++I) { + ArrayRef TokRef(Tokens); hokein wrote: > we

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

hokein added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:46 +// removed. +for (unsigned I = 0; I < Tokens.size(); ++I) { + ArrayRef TokRef(Tokens); we don't care the Kind in `HighlightingToken` now, I think we

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

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

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

jvikstrom updated this revision to Diff 210359. jvikstrom marked 2 inline comments as done. jvikstrom added a comment. Ignore tokens outside of main file. Added testcase for assert. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64741/new/

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

hokein added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:170 +return; + Loc = SM.getSpellingLoc(Loc); +} hokein wrote: > jvikstrom wrote: > > hokein wrote: > > > The Loc here maybe not in the main file,

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

ilya-biryukov added a comment. Thanks for pointing out I missed stuff. The strategy for conflicting highlightings LG, just a few NITs left from my side. In D64741#1589144 , @jvikstrom wrote: > Already added the case you sent a comment on in the test

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

jvikstrom added a comment. In D64741#1588987 , @ilya-biryukov wrote: > > I had completely missed that there could be conflicting tokens when only > > highlighting macro arguments as well. Added code to just remove conflicting > > tokens. > > Picking

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

hokein added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:170 +return; + Loc = SM.getSpellingLoc(Loc); +} jvikstrom wrote: > hokein wrote: > > The Loc here maybe not in the main file, considering the case > > >

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

ilya-biryukov added a comment. > I had completely missed that there could be conflicting tokens when only > highlighting macro arguments as well. Added code to just remove conflicting > tokens. Picking one of the highlightings looks fine, but we probably want to make sure it's deterministic.

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

jvikstrom added a comment. In D64741#1587204 , @ilya-biryukov wrote: > How should this behave when if the same token is used multiple times inside a > macro and the uses are different? Could we add this to tests? > E.g. > > #define W(a) class a {

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

jvikstrom updated this revision to Diff 210261. jvikstrom added a comment. clang-format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64741/new/ https://reviews.llvm.org/D64741 Files: clang-tools-extra/clangd/SemanticHighlighting.cpp

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

jvikstrom updated this revision to Diff 210259. jvikstrom marked an inline comment as done. jvikstrom added a comment. Fixed edge case when removing conflicting tokens. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64741/new/

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

jvikstrom marked an inline comment as done. jvikstrom added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:170 +return; + Loc = SM.getSpellingLoc(Loc); +} hokein wrote: > The Loc here maybe not in the main file,

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

jvikstrom updated this revision to Diff 210257. jvikstrom marked 2 inline comments as done. jvikstrom added a comment. Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64741/new/ https://reviews.llvm.org/D64741 Files:

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

ilya-biryukov added a comment. How should this behave when if the same token is used multiple times inside a macro and the uses are different? Could we add this to tests? E.g. #define W(a) class a { void test() { int a = 10; } } W(foo); // <-- `foo` is a variable of a class? Repository:

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

hokein added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:164 +if(Loc.isMacroID()) { + // If the location is not an argument it might be from a macro of the form + // "#define VAR var". In that case this would highlight "var" in

[PATCH] D64741: [clangd] Added highlighting for tokens that are macro arguments.

jvikstrom created this revision. jvikstrom added reviewers: hokein, sammccall, ilya-biryukov. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay. Herald added a project: clang. Adds semantic highlighting for tokens that are a macro argument. Example: #define D_V(X) int