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

[Johan Vikström via Phabricator via cfe-commits]

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:
> This is potentially `O(n^2)`. Could we instead create a new vector and fill it with the new items?
> 
> The memory usage should not matter much - we have the AST stored in the background anyway. I bet we would be looking at it first if we wanted to minimize memory usage.
> 
> If we really wanted to **not** waste any memory, we could do it `std::erase_if`-style, i.e. move the items we want to remove to the end of the vector, call `vector::erase` once at the end.
Completely forgot that erase is linear. Will go for the "allocate another vector" approach. 

Rewrote the entire conflicting checking code to hopefully be simpler as well (it changed substantially, so you might want to take another look at it).


================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:332
+      #define DEF_CLASS(T) class T {};
+      DEF_MULTIPLE(XYZ);
+      DEF_MULTIPLE(XYZW);
----------------
ilya-biryukov wrote:
> Unrelated to the change: do we plan to highlight macro names?
> That seems very useful.
Yes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64741