[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))
----------------
nridge wrote:
> 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).
> I implemented this exact highlighting in Eclipse a few years ago

I will readily admit that classifying references as mutable or not can get tricky. For example:

```
int* array = new int[10];
array[0] = 42;
```

Is the reference to `array` on the second line mutable? Does the answer change if the declaration is changed to `int array[10];`?

Fun questions to figure out later :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77811