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