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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
