This is an archive of the discontinued LLVM Phabricator instance.

[demangler] No need to space adjacent template closings
ClosedPublic

Authored by urnathan on Apr 5 2022, 8:34 AM.

Details

Summary

With the demangler parenthesizing 'a >> b' inside template parameters, because C++11 parsing of >> there, we don't really need to add spaces between adjacent template arg closing '>' chars. In 2022, that just looks odd.

Diff Detail

Event Timeline

urnathan created this revision.Apr 5 2022, 8:34 AM
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
urnathan requested review of this revision.Apr 5 2022, 8:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2022, 8:34 AM

Doesn't look like all the cases are tested? (eg: I searched for basic_iostream and I only see the source change but no test change) - I realize some of these may be previously missing test coverage, but would you mind checking/adding the missing coverage?

& I think maybe @rjmccall had signed off on this direction in another review - could you link to his comment in the patch description? (or if I'm misremembering, maybe someone else had OK'd the direction?)

I think you're right that I signed off on it; I don't remember when or where that was, or what I said about it then, but regardless I can tell you that there are two basic reasons:

  • First, the result of demangling to a string is not source code. It produces valid source code as much as it can, and that's a good choice, but there are common cases where it cannot just produce source, and there is no use case that requires it to produce source. Ultimately, this is just a string that is going to be presented to a human.
  • Since we're just going to present it to a human, what mainly matters is what the human's expectations are. Approximately no C++ programmers will be confused to see >> terminating a template argument list, in part because it's been a long since since C++11 was available, but also simply because it was unnatural for programmers to remember to include the space in the first place, to the point that any reasonable compiler had to catch it and recover as a common error.

If you want to put that in the commit, feel free.

This makes sense. In addition, clang's type printer has omitted space in >> since 2021 (a change by sammccall).

urnathan updated this revision to Diff 427665.May 6 2022, 9:55 AM

Added special subst tests

MaskRay accepted this revision.May 7 2022, 12:34 AM
This revision is now accepted and ready to land.May 7 2022, 12:34 AM
This revision was landed with ongoing or failed builds.May 9 2022, 6:15 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2022, 6:15 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
libcxxabi/src/demangle/ItaniumDemangle.h