This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Avoid unexpected desugaring in isSugaredTemplateParameter

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



This is a follow-up to D151785, addressing

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

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


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

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

Nice find, this is indeed pretty tricky. I remember running into a similar issue before in

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


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.


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


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



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.