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

[Ilya Biryukov via Phabricator via cfe-commits]

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 <https://reviews.llvm.org/D64741#1589144>, @jvikstrom wrote:

> Already added the case you sent a comment on in the test case. (look at the top of it)


Ah, sorry, I missed it. There are many there and the one you added slipped my mind.

> Don't understand what you mean with the `sort` though. Kinds are already included in the comparator for sort. After the call to unique the only tokens that will share ranges are the ones that have different kinds.

Ah, right. Sorry, missed that as well, `sort()` LG, the results are deterministic.

> Could just have a custom comparator in the call to unique that only compares the tokens' ranges which would leave us with the token whose kind is the lowest when there are conflicting ones. But do we really want to highlight anything when there are conflicts?
>  Maybe we should add another kind of Kind for when a token is conflicting?

Not highlighting for conflicting kinds LG (there is one interesting case that I think we should support at the end of my comment, other than it looks ok).
Conflicting would be hard to explain to users, so not having it look like a better option.

Now, one interesting case that we should probably support is `assert`. Schematically, it does something like:

  #define assert(COND) if (!(cond)) { fail("assertion failed" #COND); }
  assert(x != y);

Despite the argument being mentioned twice, we definitely only want highlightings from the first occurrence (that would give us kinds for expressions).
I would expect that this already works, in which case it's just a matter of adding a test case to guard against future regressions.

If this does not work, we should probably fix it in a separate change.



================
Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:195
+      #define DEF_CLASS(T) class T {};
+      DEF_MULTIPLE(XYZ);
+      DEF_MULTIPLE(XYZW);
----------------
Could you add a comment explaining that we choose to not highlight the conflicting tokens?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64741