Page MenuHomePhabricator

[clangd] Add "usedAsMutablePointer" highlighting modifier
Needs ReviewPublic

Authored by ckandeler on Jul 18 2022, 8:06 AM.

Details

Summary

Counterpart to "usedAsMutableReference". Just as for references, there
are const and non-const pointer parameters, and it's valuable to be able
to have different highlighting for the two cases at the call site.
We could have re-used the existing modifier, but having a dedicated one
maximizes client flexibility.

Diff Detail

Event Timeline

ckandeler created this revision.Jul 18 2022, 8:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2022, 8:06 AM
ckandeler requested review of this revision.Jul 18 2022, 8:06 AM
nridge added a comment.Aug 4 2022, 2:56 PM

Sorry, I'm travelling and probably won't be able to respond promptly, but my first thought here is that this doesn't seem as well motivated as usedAsMutableReference, since taking a pointer to an object involves using explicit syntax (&obj) that taking a reference does not. My mental model of usedAsMutableReference is that it's a workaround for the fact that C++ does not have explicit syntax for forming a mutable reference (compare Rust's ref obj which in my mind makes this modifier likewise unnecessary there).

IMO the relevant point is the (non-)const-ness of the corresponding parameter, i.e. can the passed object get mutated or not. The fact that there's an ampersand at the call site (which is not even the case if the variable is itself a pointer) does not tell me that.

I agree with Nathan to some extent that this doesn't carry as much weight, but if we can get it without introducing much complexity to the user (i.e. a new modifier they need to remember), it's worth having since we don't need too much effort to support it.

clang-tools-extra/clangd/SemanticHighlighting.cpp
578–581

nit: we don't do consts for locals (not saying this is a bad practice, it's just not inline with the convention in the rest of the codebase).

603–604

why do you think we should have different modifiers for these? they both indicate the same thing, the parameter might get mutated by the call, and it being a pointer or reference doesn't seem to matter.

ckandeler updated this revision to Diff 451420.Aug 10 2022, 4:54 AM
ckandeler marked an inline comment as done.

Fixed style issues.

clang-tools-extra/clangd/SemanticHighlighting.cpp
603–604

Because I'm aware that there are people (like Nathan) who don't find it useful for pointers, and having just one modifier would force it on everybody. With two distinct modifiers, clients can decide for themselves, possibly even exposing the choice to users.