Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[clangd] Add "readonly" token to const member expressions
Needs ReviewPublic

Authored by t-troebst on Apr 13 2023, 6:20 PM.



In semantic highlighting: this adds the readonly token to a in an member expression of the form`a` (implicit this), (...).a or (...)->a if a is const in this context. Previously, only the type of the declaration of a was considered.

Diff Detail

Event Timeline

t-troebst created this revision.Apr 13 2023, 6:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2023, 6:20 PM
t-troebst requested review of this revision.Apr 13 2023, 6:20 PM
t-troebst updated this revision to Diff 513406.Apr 13 2023, 6:29 PM

Changed isConstQualifed() to the custom isConst() (though this isConst() behaves strangely for pointer types, we should fix this separately and keep them synced for now).

Thanks for the patch!

addExtraModifier is a kind of hacky mechanism to attach modifiers in CollectExtraHighlightings to tokens created during findExplicitReferences(); it would be nicer to add the modifier at the time of creating the token, but it doesn't look like we have enough information in ReferenceLoc to do that in this case. We could consider extending ReferenceLoc with more information in the future, but I think this is fine for now.

Do you think it's feasible to make a best-effort attempt to use similar logic for CXXDependentScopeMemberExpr? I don't think it returns a useful getType(), but its getBaseType() should tell us whether the object type is const or not. (It wouldn't handle mutable because we don't have a concrete member decl to work with, but that's a less common case compared to the base type being const or not.)

t-troebst updated this revision to Diff 518641.May 1 2023, 11:18 PM

Add best effort attempt to get const-ness for dependent member expressions.

I've added some logic to get const-ness for dependent expressions. I think you're right that its probably reasonable to do this since mutable is almost always used in member functions where the type isn't dependent anyways.

nridge added inline comments.May 17 2023, 11:10 PM

getPointeeType() needs a "null" check (check for empty QualType), this can arise if e.g. BaseType is a smart pointer type (E->isArrow() will still be true because the expression syntactically uses ->, but BaseType will not be a PointerType)

Otherwise looks good, thanks!