This is an archive of the discontinued LLVM Phabricator instance.

Differential D122474

demangler] Parenthesize >> inside template args
ClosedPublic

Authored by urnathan on Mar 25 2022, 4:40 AM.

Details

Reviewers
rjmccall
bruno
dblaikie
Group Reviewers
Restricted Project
Commits
rGee6ec9e861e6: [demangler] Parenthesize >> inside template args
Summary

Both > and >> expressions need to be parenthesized inside template argument lists.

(as noted in D120905)

Any opinions about removing the insertion of ' ' between consecutive template arg closing '>' chars? tpl<tpl<x>> vs tpl<tpl<x> >

Diff Detail

Unit TestsFailed

TimeTest
60,030 msx64 debian > MLIR.Examples/standalone::test.toy
60,080 msx64 debian > libFuzzer.libFuzzer::large.test

Event Timeline

urnathan created this revision.Mar 25 2022, 4:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2022, 4:40 AM
urnathan requested review of this revision.Mar 25 2022, 4:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2022, 4:40 AM
Herald added a subscriber: llvm-commits. · View Herald Transcript
urnathan added a parent revision: D120905: [demangler] Add operator precedence.Mar 25 2022, 4:41 AM
urnathan edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B156256: Diff 418186.Mar 25 2022, 5:56 AM
rjmccall added a comment.Mar 25 2022, 10:12 PM

I think C++11 is pervasive enough that removing the space is fine. Demangler output is not necessarily valid source anyway.

Patch LGTM.

urnathan updated this revision to Diff 418656.Mar 28 2022, 11:22 AM

rebase

Harbormaster completed remote builds in B156611: Diff 418656.Mar 29 2022, 3:40 PM
urnathan added a comment.Mar 31 2022, 5:28 AM
In D122474#3409394, @rjmccall wrote:

I think C++11 is pervasive enough that removing the space is fine. Demangler output is not necessarily valid source anyway.

Patch LGTM.

I'll take that as approval, unless someone chimes in in the next couple of days.

urnathan added a reviewer: Restricted Project.Mar 31 2022, 8:44 AM
dblaikie accepted this revision.Mar 31 2022, 1:42 PM
In D122474#3419012, @urnathan wrote:
In D122474#3409394, @rjmccall wrote:

I think C++11 is pervasive enough that removing the space is fine. Demangler output is not necessarily valid source anyway.

Patch LGTM.

I'll take that as approval, unless someone chimes in in the next couple of days.

Yep, sounds good!

This revision is now accepted and ready to land.Mar 31 2022, 1:42 PM
This revision was landed with ongoing or failed builds.Apr 4 2022, 6:36 AM
Closed by commit rGee6ec9e861e6: [demangler] Parenthesize >> inside template args (authored by urnathan). · Explain Why
This revision was automatically updated to reflect the committed changes.
urnathan added a commit: rGee6ec9e861e6: [demangler] Parenthesize >> inside template args.
Herald added a project: Restricted Project. · View Herald TranscriptApr 4 2022, 6:36 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
Herald added a subscriber: libcxx-commits. · View Herald Transcript
urnathan mentioned this in D123134: [demangler] No need to space adjacent template closings.May 6 2022, 4:49 AM

Revision Contents

PathSize
libcxxabi/
src/
demangle/
3 lines
test/
2 lines
llvm/
include/
llvm/
Demangle/
3 lines

Diff 418186

libcxxabi/src/demangle/ItaniumDemangle.h

Show First 20 Lines • Show All 1,779 Lines • ▼ Show 20 Linespublic:
BinaryExpr(const Node *LHS_, StringView InfixOperator_, const Node *RHS_, BinaryExpr(const Node *LHS_, StringView InfixOperator_, const Node *RHS_,
Prec Prec_) Prec Prec_)
: Node(KBinaryExpr, Prec_), LHS(LHS_), InfixOperator(InfixOperator_), : Node(KBinaryExpr, Prec_), LHS(LHS_), InfixOperator(InfixOperator_),
RHS(RHS_) {} RHS(RHS_) {}
template<typename Fn> void match(Fn F) const { F(LHS, InfixOperator, RHS); } template<typename Fn> void match(Fn F) const { F(LHS, InfixOperator, RHS); }
void printLeft(OutputBuffer &OB) const override { void printLeft(OutputBuffer &OB) const override {
bool ParenAll = OB.isGtInsideTemplateArgs() && InfixOperator == ">"; bool ParenAll = OB.isGtInsideTemplateArgs() &&
(InfixOperator == ">" || InfixOperator == ">>");
if (ParenAll) if (ParenAll)
OB.printOpen(); OB.printOpen();
// Assignment is right associative, with special LHS precedence. // Assignment is right associative, with special LHS precedence.
bool IsAssign = getPrecedence() == Prec::Assign; bool IsAssign = getPrecedence() == Prec::Assign;
LHS->printAsOperand(OB, IsAssign ? Prec::OrIf : getPrecedence(), !IsAssign); LHS->printAsOperand(OB, IsAssign ? Prec::OrIf : getPrecedence(), !IsAssign);
// No space before comma operator // No space before comma operator
if (!(InfixOperator == ",")) if (!(InfixOperator == ","))
OB += " "; OB += " ";
▲ Show 20 Lines • Show All 3,752 Lines • Show Last 20 Lines

libcxxabi/test/test_demangle.pass.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 29,623 Lines • ▼ Show 20 Lines
"void Casts::implicit<4u>(enable_if<4u->4, void>::type*)"}, "void Casts::implicit<4u>(enable_if<4u->4, void>::type*)"},
{"_ZN5Casts8implicitILj4EEEvPN9enable_ifIXquT_Li4ELi5EEvE4typeE", {"_ZN5Casts8implicitILj4EEEvPN9enable_ifIXquT_Li4ELi5EEvE4typeE",
"void Casts::implicit<4u>(enable_if<4u ? 4 : 5, void>::type*)"}, "void Casts::implicit<4u>(enable_if<4u ? 4 : 5, void>::type*)"},
{"_ZN5Casts8implicitILj4EEEvPN9enable_ifIXrmT_Li4EEvE4typeE", {"_ZN5Casts8implicitILj4EEEvPN9enable_ifIXrmT_Li4EEvE4typeE",
"void Casts::implicit<4u>(enable_if<4u % 4, void>::type*)"}, "void Casts::implicit<4u>(enable_if<4u % 4, void>::type*)"},
{"_ZN5Casts8implicitILj4EEEvPN9enable_ifIXrMT_Li4EEvE4typeE", {"_ZN5Casts8implicitILj4EEEvPN9enable_ifIXrMT_Li4EEvE4typeE",
"void Casts::implicit<4u>(enable_if<4u %= 4, void>::type*)"}, "void Casts::implicit<4u>(enable_if<4u %= 4, void>::type*)"},
{"_ZN5Casts8implicitILj4EEEvPN9enable_ifIXrsT_Li4EEvE4typeE", {"_ZN5Casts8implicitILj4EEEvPN9enable_ifIXrsT_Li4EEvE4typeE",
"void Casts::implicit<4u>(enable_if<4u >> 4, void>::type*)"}, "void Casts::implicit<4u>(enable_if<(4u >> 4), void>::type*)"},
{"_ZN5Casts8implicitILj4EEEvPN9enable_ifIXrST_Li4EEvE4typeE", {"_ZN5Casts8implicitILj4EEEvPN9enable_ifIXrST_Li4EEvE4typeE",
"void Casts::implicit<4u>(enable_if<4u >>= 4, void>::type*)"}, "void Casts::implicit<4u>(enable_if<4u >>= 4, void>::type*)"},
{"_Z1fPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPP1XS13_S12_S11_S10_SZ_SY_SX_SW_SV_SU_ST_SS_SR_SQ_SP_SO_SN_SM_SL_SK_SJ_" {"_Z1fPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPPP1XS13_S12_S11_S10_SZ_SY_SX_SW_SV_SU_ST_SS_SR_SQ_SP_SO_SN_SM_SL_SK_SJ_"
"SI_SH_SG_SF_SE_SD_SC_SB_SA_S9_S8_S7_S6_S5_S4_S3_S2_S1_S0_S_", "SI_SH_SG_SF_SE_SD_SC_SB_SA_S9_S8_S7_S6_S5_S4_S3_S2_S1_S0_S_",
"f(X****************************************, X****************************************, " "f(X****************************************, X****************************************, "
"X***************************************, X**************************************, " "X***************************************, X**************************************, "
"X*************************************, X************************************, " "X*************************************, X************************************, "
"X***********************************, X**********************************, X*********************************, " "X***********************************, X**********************************, X*********************************, "
▲ Show 20 Lines • Show All 698 Lines • Show Last 20 Lines

llvm/include/llvm/Demangle/ItaniumDemangle.h

Show First 20 Lines • Show All 1,779 Lines • ▼ Show 20 Linespublic:
BinaryExpr(const Node *LHS_, StringView InfixOperator_, const Node *RHS_, BinaryExpr(const Node *LHS_, StringView InfixOperator_, const Node *RHS_,
Prec Prec_) Prec Prec_)
: Node(KBinaryExpr, Prec_), LHS(LHS_), InfixOperator(InfixOperator_), : Node(KBinaryExpr, Prec_), LHS(LHS_), InfixOperator(InfixOperator_),
RHS(RHS_) {} RHS(RHS_) {}
template<typename Fn> void match(Fn F) const { F(LHS, InfixOperator, RHS); } template<typename Fn> void match(Fn F) const { F(LHS, InfixOperator, RHS); }
void printLeft(OutputBuffer &OB) const override { void printLeft(OutputBuffer &OB) const override {
bool ParenAll = OB.isGtInsideTemplateArgs() && InfixOperator == ">"; bool ParenAll = OB.isGtInsideTemplateArgs() &&
(InfixOperator == ">" || InfixOperator == ">>");
if (ParenAll) if (ParenAll)
OB.printOpen(); OB.printOpen();
// Assignment is right associative, with special LHS precedence. // Assignment is right associative, with special LHS precedence.
bool IsAssign = getPrecedence() == Prec::Assign; bool IsAssign = getPrecedence() == Prec::Assign;
LHS->printAsOperand(OB, IsAssign ? Prec::OrIf : getPrecedence(), !IsAssign); LHS->printAsOperand(OB, IsAssign ? Prec::OrIf : getPrecedence(), !IsAssign);
// No space before comma operator // No space before comma operator
if (!(InfixOperator == ",")) if (!(InfixOperator == ","))
OB += " "; OB += " ";
▲ Show 20 Lines • Show All 3,752 Lines • Show Last 20 Lines