This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add "usedAsMutablePointer" highlighting modifier
ClosedPublic

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

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

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

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.

nridge added a comment.Nov 5 2022, 1:21 AM

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.

Taking another look at this, I find this use case convincing and I'm fine with taking this patch.

I do prefer the separate modifier for customizability, as I think the desirability of highlighting this can vary based on the coding convention. For example, one project I work with is slightly older and a lot of objects are passed around by non-const pointer, but actual "out parameters" almost always take the form of &var at the call site or use a reference; in such a case, the pointer modifier would be fairly noisy and it's nice to be able to disable it while keeping the reference modifier on.

clang-tools-extra/clangd/SemanticHighlighting.cpp
587

nit: you can remove the "and UnaryOperator" part of the FIXME, since you're addressing it

592

Could you add a test case that exercises the UnaryOperator case?

ckandeler added inline comments.Nov 7 2022, 1:39 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
592

Doesn't the very first hunk in SemanticHighlightingTests.cpp do that?

nridge accepted this revision.Nov 7 2022, 1:43 AM
nridge added inline comments.
clang-tools-extra/clangd/SemanticHighlighting.cpp
592

Yep, my bad, overlooked that

This revision is now accepted and ready to land.Nov 7 2022, 1:43 AM
ckandeler updated this revision to Diff 473603.Nov 7 2022, 2:05 AM
ckandeler marked an inline comment as done.

Rebased and updated comment as indicated in review.