This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Parameter hints for calls through function pointers
ClosedPublic

Authored by nridge on Aug 17 2023, 10:57 PM.

Diff Detail

Event Timeline

nridge created this revision.Aug 17 2023, 10:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2023, 10:57 PM
nridge requested review of this revision.Aug 17 2023, 10:57 PM
nridge edited the summary of this revision. (Show Details)Aug 17 2023, 10:57 PM
nridge added inline comments.Aug 17 2023, 11:03 PM
clang-tools-extra/clangd/InlayHints.cpp
466

The reason I didn't unify them in this patch is less that I'm lazy, and more that I'm not sure where to put code that will be shared by SemaCodeComplete.cpp and clangd.

Do we have some sort of dumping ground for miscellaneous AST utilities, like clang-tools-extra/clangd/AST.h but also usable in libSema?

nridge updated this revision to Diff 551390.Aug 17 2023, 11:12 PM

Fix naming nit

nridge added inline comments.Aug 17 2023, 11:14 PM
clang-tools-extra/clangd/InlayHints.cpp
466

I guess the function is performing a heuristic / best-effort operation, so HeuristicResolver could be the place for it once that is upstreamed.

sammccall accepted this revision.Aug 18 2023, 2:07 AM
sammccall added inline comments.
clang-tools-extra/clangd/InlayHints.cpp
508

Calling this "callee" confuses me a bit - originally the callee was an expr and always present, so this must be a different idea: CalleeDecl?.

However I also think this struct has an unneccesarily broad scope: we're not really gaining anything by bundling Args into it rather than continuing to pass that as a separate param.
This suggests:

struct Callee {
  Decl *Declaration;
  FunctionProtoTypeLoc Loc;
};
This revision is now accepted and ready to land.Aug 18 2023, 2:07 AM
sammccall added inline comments.Aug 18 2023, 2:12 AM
clang-tools-extra/clangd/InlayHints.cpp
466

(I'm happy with this being where it is, for lack of a really good option)

I don't have a good answer here - sometimes people seem to dumb these as methods on Expr etc itself, but that seems terrible. AST is probably the best existing library for it if we want to use it from Sema, but it feels a little... impure.

It doesn't feel like a perfect fit for HeuristicResolver but in practice if we're going to move that into AST then it seems best overall for this function to come along for the ride.

nridge updated this revision to Diff 551696.Aug 18 2023, 8:39 PM

Address review comments

nridge marked an inline comment as done.Aug 18 2023, 8:40 PM
nridge added inline comments.
clang-tools-extra/clangd/InlayHints.cpp
508

Good point, updated

This revision was landed with ongoing or failed builds.Aug 18 2023, 8:41 PM
This revision was automatically updated to reflect the committed changes.
nridge marked an inline comment as done.