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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to