sammccall marked an inline comment as done. sammccall added inline comments.
================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:239 + auto Result = Priority1 > Priority2 ? Tokens[0] : Tokens[1]; + Result.Modifiers = Tokens[0].Modifiers | Tokens[1].Modifiers; + return Result; ---------------- nridge wrote: > I like that this ends up with a concrete kind + dependent modifier, to give a > hint that the concrete kind was determined heuristically :) > > Maybe we could adjust the comment at the top of the function to call this > behaviour out. Right! I like this too but forgot do doc it. Done. ================ Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:78 Abstract, + DependentName, ---------------- nridge wrote: > Just `Dependent` might be enough AFAICS the current implementation is closer to "dependent name" than "dependent", I think. For example, in the RHS of `template <typename T> using Val = T::value_type`, both "T" and "value_type" certainly refer to dependent types. But only `value_type` is a dependent name, and only `value_type` gets the old kinds/new modifier. --- Or are you proposing we change the implementation too? It's not intuitively which version would be more useful. FWIW I have the current DependentName kind highlighted in bold bright orange, and I find it really helpful :-) When there are errors and RecoveryExpr kicks in, it colors the resulting unresolved names, which I like surprisingly much. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95706/new/ https://reviews.llvm.org/D95706 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
