This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Hide inlay hints when using a macro as a calling argument that with a param comment

Authored by zyounan on Feb 14 2023, 11:58 PM.



We don't want to produce inlay hints for arguments for which
user has left param name comments. But we're not decomposing
location of the parameter correctly at the moment because the
location we've passed into SM.getDecomposedLoc is not always

Fixes clangd/clangd#1495

Diff Detail

Event Timeline

zyounan created this revision.Feb 14 2023, 11:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2023, 11:58 PM
zyounan added a comment.EditedFeb 15 2023, 12:12 AM

Note that we still have some issues on inlay hints with macro expansion even after D133982.

void func(int a, int b);
#define CALL(a, b) func(int(a), b)

CALL(3, 4);  // CALL(3, b: 4)

This is because spelledForExpanded doesn't return counterpart spelling ( int(3) -> 3 ). (I guess it is due to patch D134618.)

void func(int a, int b);
void work(int a, int &b);
#define CALL(a, b) { func(int(a), b); work(a, b); }

int z = 42;
CALL(3, z);  // CALL(a: 3, &b:b: 4)

This is because call expression handling logic (i.e. InlayHintVisitor::processCall) doesn't know the macro expansion context of expression, resulting producing two inlay hints at the same spelled position.

zyounan retitled this revision from [clangd] Hide extra inlay hints for macro as argument to [clangd] Hide inlay hints when using a macro as a calling argument that with a param comment.Feb 15 2023, 12:25 AM
zyounan published this revision for review.Feb 15 2023, 5:51 AM
zyounan added inline comments.

stupid question: I was suspicious with this check that when would Decomposed.first not being the same as MainFileID? Should the "top-caller" of the macro always be in main file? I didn't find a case that differs, except when getDecomposedLoc provides wrong FileID.

nridge added inline comments.Feb 23 2023, 10:31 PM

I admit I struggle with this spelling/expansion loc stuff. But having spent a bit of time reading the implementations of these functions, would I be correct to say that another, perhaps easier to follow, way to express the same thing would be:

auto Decomposed = SM.getDecomposedLoc(SM.getFileLoc(E->getBeginLoc()));



I tried adding an assertion in this early-return branch, and it tripped in ParameterHints.IncludeAtNonGlobalScope.

The code there is:

Annotations FooInc(R"cpp(
  void bar() { foo(42); }
Annotations FooCC(R"cpp(
  struct S {
    void foo(int param);
    #include ""

so I guess the decomposed loc there is a file loc, but not in the main file (but rather in

zyounan updated this revision to Diff 500387.Feb 25 2023, 1:05 AM
zyounan marked an inline comment as done.

Address the comments


Yes, indeed. SM.getFileLoc performs exactly the same function as getTopMacroCallerLoc and getDecomposedExpansionLoc:
If Loc is a FileID, just return Loc.
In another case it falls into a loop until the result is a FileID: First obtain the "top macro caller" location, then the beginning expansion location.


Ah, I see. Thanks!

zyounan marked an inline comment as done.Feb 25 2023, 1:07 AM
nridge accepted this revision.Feb 25 2023, 7:06 PM


This revision is now accepted and ready to land.Feb 25 2023, 7:06 PM