This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Avoid unexpected desugaring in isSugaredTemplateParameter
ClosedPublic

Authored by zyounan on Jul 26 2023, 1:14 AM.

Details

Summary

This is a follow-up to D151785, addressing https://github.com/clangd/clangd/issues/1703.

The previous approach of peeling pointer types during a traversal
using getPointeeType might have produced unexpected results; since
the method would implicitly desugar the type if the type being passed
in could not be cast to a Pointer-like Type.

Diff Detail

Event Timeline

zyounan created this revision.Jul 26 2023, 1:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2023, 1:14 AM
zyounan requested review of this revision.Jul 26 2023, 1:14 AM

Your patience is appreciated. I have a number of patches in my review queue, and 3 days is often not a realistic turnaround time for me.

Oops, my apologies for bothering you. Thanks again for the explanation and your dedication!

nridge added inline comments.Jul 31 2023, 1:12 AM
clang-tools-extra/clangd/InlayHints.cpp
198

nit: since it's only doing one level of peeling, let's call it PeelWrapper (no s)

202

Now that the function is not looping, we can simplify the body a bit:

QualType Peeled = QT->getPointeeType();
return Peeled.isNull() ? QT : Peeled;
207

Nice find, this is indeed pretty tricky. I remember running into a similar issue before in https://reviews.llvm.org/D124690#inline-1205484.

zyounan updated this revision to Diff 545920.Jul 31 2023, 11:06 PM
zyounan marked 2 inline comments as done.

Update

Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2023, 11:06 PM
zyounan updated this revision to Diff 545922.Jul 31 2023, 11:09 PM

Fix the chaos in the previous update

Sorry, looks like I messed up with the commits before. Fixed it now.

clang-tools-extra/clangd/InlayHints.cpp
207

Cool. That looks far more intricate and I should take some time to understand the context. Thanks for the reference :)

clang-tools-extra/clangd/unittests/InlayHintTests.cpp
1431–1432

I feel like this test is becoming clunky: The validations here require an invented class template vector which I think should be better placed in a header; however, in the remaining snippet, it seems that I'm testing too many hints in one assertHint call. Should I split these cases into different snippets, or we just leave them as-is at the moment?

nridge accepted this revision.Jul 31 2023, 11:48 PM

Thanks!

clang-tools-extra/clangd/unittests/InlayHintTests.cpp
1431–1432

I think this is fine as-is for now

This revision is now accepted and ready to land.Jul 31 2023, 11:48 PM
zyounan updated this revision to Diff 546008.Aug 1 2023, 5:05 AM

Final update

This revision was landed with ongoing or failed builds.Aug 1 2023, 5:12 AM
This revision was automatically updated to reflect the committed changes.