[PATCH] D77811: [clangd] Implement semanticTokens modifiers

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:136
+// Whether D is const in a loose sense (should it be highlighted as such?)
+bool isConst(const Decl *D) {
+  if (llvm::isa<EnumConstantDecl>(D) || llvm::isa<NonTypeTemplateParmDecl>(D))
----------------
sammccall wrote:
> nridge wrote:
> > Do you think in the future it might make sense to have the `readonly` modifier reflect, at least for variables, whether //the particular reference// to the variable is a readonly reference (e.g. binding the variable to a `const X&` vs. an `X&`)?
> Do I understand correctly?
> 
> ```
> std::vector<int> X; // X is not readonly
> X.push_back(42); // X is not readonly
> X.size(); // X is readonly
> ```
> 
> Distinguishing potentially-mutating from non-mutating uses seems really useful to me.
> My only question is whether mapping this concept onto `readonly` is the right thing to do:
>  - there are really three sets: const access, mutable access, and non-access (e.g. declaration, or on RHS of `using a=b`). And I think maybe the most useful thing to distinguish is mutable access vs everything else, which is awkward with an attribute called "readonly".
>  - this also seems likely to diverge from how other languages use the attribute (most don't have this concept)
>  - on the other hand, standard attribute names will be better supported by clients
> 
> This also might be somewhat complicated to implement :-)
> I'd like to leave in this simple decl-based thing as a placeholder, and either we can replace it and add an additional "mutation" attribute later (once we work out how to implement it!) I've left a comment about this...
> 
> (Related: a while ago Google's style guide dropped its requirement to use pointers rather than references for mutable function params. Having `&x` at the callsite rather than just `x` is a useful hint, but obviously diverging from common practice has a cost. We discussed how we could use semantic highlighting to highlight where a param was being passed by mutable reference, though didn't have client-side support for it yet)
> Do I understand correctly?
> 
> ```
> std::vector<int> X; // X is not readonly
> X.push_back(42); // X is not readonly
> X.size(); // X is readonly
> ```

Yup, that's what I had in mind.

> Distinguishing potentially-mutating from non-mutating uses seems really useful to me.
> My only question is whether mapping this concept onto `readonly` is the right thing to do:
>  - there are really three sets: const access, mutable access, and non-access (e.g. declaration, or on RHS of `using a=b`). And I think maybe the most useful thing to distinguish is mutable access vs everything else, which is awkward with an attribute called "readonly".
>  - this also seems likely to diverge from how other languages use the attribute (most don't have this concept)
>  - on the other hand, standard attribute names will be better supported by clients
> 
> This also might be somewhat complicated to implement :-)
> I'd like to leave in this simple decl-based thing as a placeholder, and either we can replace it and add an additional "mutation" attribute later (once we work out how to implement it!) I've left a comment about this...

Sounds good!

> (Related: a while ago Google's style guide dropped its requirement to use pointers rather than references for mutable function params. Having `&x` at the callsite rather than just `x` is a useful hint, but obviously diverging from common practice has a cost. We discussed how we could use semantic highlighting to highlight where a param was being passed by mutable reference, though didn't have client-side support for it yet)

Funny you mention this, because I implemented this exact highlighting in Eclipse a few years ago:

https://wiki.eclipse.org/CDT/User/NewIn92#Syntax_coloring_for_variables_passed_by_non-const_reference

(and it's my main motivation for the suggestion I've made here).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77811