Page MenuHomePhabricator

[clangd] fix crash and handle implicit conversions in inlay hints for forwarding functions
AbandonedPublic

Authored by upsj on Jul 21 2022, 5:42 AM.

Details

Summary

The ForwardingCallVisitor previously picked up on (constructor or function) calls inside a single argument of a variadic function, where the corresponding pack expression is (partially) expanded, but only consists of a single entry. This mismatch between expecting the full pack and finding only a single entry caused a crash, which the first half of this diff fixes.

The second half deals with the failure of resolveForwardingParameters to detect forwarded parameters when implicit conversions happen via constructor calls, not normal ImplicitCastExpr.

Diff Detail

Event Timeline

upsj created this revision.Jul 21 2022, 5:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 5:42 AM
upsj requested review of this revision.Jul 21 2022, 5:42 AM
upsj added inline comments.Jul 21 2022, 5:47 AM
clang-tools-extra/clangd/unittests/InlayHintTests.cpp
721

I'm not sure whether we should detect this case, since it would be nice to handle, but also involves digging deep through CXXConstructExpression -> MaterializeTemporaryExpression -> CXXMemberCallExpr -> MemberExpr -> DeclRefExpr

sorry for not mentioning in the bug that i was also working on a patch, D130260 seems to be a little bit more contained and focused on making the check not crash (as we're getting close to release cut).

WDYT about landing it now, and iterating on this patch afterwards if you still have cases that it doesn't handle but this patch does? (I can't really see such cases in hindsight)

clang-tools-extra/clangd/AST.cpp
844

so in theory this is still a heuristic, and somewhat complicated. what about just checking if we have "enough parameters" to satisfy the pack expansion (as i did in D130260)

914

this is already handled by IgnoreImplicitAsWritten the underlying function is using now (landed in https://reviews.llvm.org/D130261)

919

this is checking for constructor being explicit, and not the callexpr itself. so for example it won't fire up if call is implicit to an explicitly defined copy constructor.

sorry for not mentioning in the bug that i was also working on a patch,

Which bug/issue is this?