[PATCH] D48063: [clang-format] Discourage breaks in submessage entries, hard rule
This revision was automatically updated to reflect the committed changes. Closed by commit rL334517: [clang-format] Discourage breaks in submessage entries, hard rule (authored by krasimir, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D48063 Files: cfe/trunk/lib/Format/TokenAnnotator.cpp cfe/trunk/unittests/Format/FormatTestProto.cpp cfe/trunk/unittests/Format/FormatTestTextProto.cpp Index: cfe/trunk/lib/Format/TokenAnnotator.cpp === --- cfe/trunk/lib/Format/TokenAnnotator.cpp +++ cfe/trunk/lib/Format/TokenAnnotator.cpp @@ -3101,10 +3101,39 @@ !Right.isOneOf(TT_CtorInitializerColon, TT_InlineASMColon)) return false; if (Left.is(tok::colon) && Left.isOneOf(TT_DictLiteral, TT_ObjCMethodExpr)) { -if ((Style.Language == FormatStyle::LK_Proto || - Style.Language == FormatStyle::LK_TextProto) && -!Style.AlwaysBreakBeforeMultilineStrings && Right.isStringLiteral()) - return false; +if (Style.Language == FormatStyle::LK_Proto || +Style.Language == FormatStyle::LK_TextProto) { + if (!Style.AlwaysBreakBeforeMultilineStrings && Right.isStringLiteral()) +return false; + // Prevent cases like: + // + // submessage: + // { key: valu } + // + // when the snippet does not fit into one line. + // Prefer: + // + // submessage: { + // key: valu + // } + // + // instead, even if it is longer by one line. + // + // Note that this allows allows the "{" to go over the column limit + // when the column limit is just between ":" and "{", but that does + // not happen too often and alternative formattings in this case are + // not much better. + // + // The code covers the cases: + // + // submessage: { ... } + // submessage: < ... > + // repeated: [ ... ] + if (((Right.is(tok::l_brace) || Right.is(tok::less)) && + Right.is(TT_DictLiteral)) || + Right.is(TT_ArrayInitializerLSquare)) +return false; +} return true; } if (Right.is(tok::r_square) && Right.MatchingParen && Index: cfe/trunk/unittests/Format/FormatTestTextProto.cpp === --- cfe/trunk/unittests/Format/FormatTestTextProto.cpp +++ cfe/trunk/unittests/Format/FormatTestTextProto.cpp @@ -485,8 +485,15 @@ verifyFormat("keys: []"); verifyFormat("keys: [ 1 ]"); verifyFormat("keys: [ 'ala', 'bala' ]"); - verifyFormat("keys:\n" - "[ 'ala', 'bala', 'porto', 'kala', 'too', 'long', 'ng' ]"); + verifyFormat("keys: [\n" + " 'ala',\n" + " 'bala',\n" + " 'porto',\n" + " 'kala',\n" + " 'too',\n" + " 'long',\n" + " 'ng'\n" + "]"); verifyFormat("key: item\n" "keys: [\n" " 'ala',\n" @@ -670,5 +677,28 @@ "}"); } +TEST_F(FormatTestTextProto, PreventBreaksBetweenKeyAndSubmessages) { + verifyFormat("submessage: {\n" + " key: 'a'\n" + "}"); + verifyFormat("submessage {\n" + " key: 'a'\n" + "}"); + verifyFormat("submessage: <\n" + " key: 'a'\n" + ">"); + verifyFormat("submessage <\n" + " key: 'a'\n" + ">"); + verifyFormat("repeatedd: [\n" + " 'ey'\n" + "]"); + // "{" is going over the column limit. + verifyFormat( + "submessagee: {\n" + " key: 'a'\n" + "}"); +} + } // end namespace tooling } // end namespace clang Index: cfe/trunk/unittests/Format/FormatTestProto.cpp === --- cfe/trunk/unittests/Format/FormatTestProto.cpp +++ cfe/trunk/unittests/Format/FormatTestProto.cpp @@ -624,5 +624,34 @@ "}"); } +TEST_F(FormatTestProto, PreventBreaksBetweenKeyAndSubmessages) { + verifyFormat("option (MyProto.options) = {\n" + " submessage: {\n" + "key: 'a'\n" + " }\n" + "}"); + verifyFormat("option (MyProto.options) = {\n" + " submessage {\n" + "key: 'a'\n" + " }\n" + "}"); + verifyFormat("option (MyProto.options) = {\n" + " submessage: <\n" + "key: 'a'\n"
[PATCH] D48063: [clang-format] Discourage breaks in submessage entries, hard rule
krasimir updated this revision to Diff 150980. krasimir added a comment. - Split-up the if-condition Repository: rC Clang https://reviews.llvm.org/D48063 Files: lib/Format/TokenAnnotator.cpp unittests/Format/FormatTestProto.cpp unittests/Format/FormatTestTextProto.cpp Index: unittests/Format/FormatTestTextProto.cpp === --- unittests/Format/FormatTestTextProto.cpp +++ unittests/Format/FormatTestTextProto.cpp @@ -485,8 +485,15 @@ verifyFormat("keys: []"); verifyFormat("keys: [ 1 ]"); verifyFormat("keys: [ 'ala', 'bala' ]"); - verifyFormat("keys:\n" - "[ 'ala', 'bala', 'porto', 'kala', 'too', 'long', 'ng' ]"); + verifyFormat("keys: [\n" + " 'ala',\n" + " 'bala',\n" + " 'porto',\n" + " 'kala',\n" + " 'too',\n" + " 'long',\n" + " 'ng'\n" + "]"); verifyFormat("key: item\n" "keys: [\n" " 'ala',\n" @@ -670,5 +677,28 @@ "}"); } +TEST_F(FormatTestTextProto, PreventBreaksBetweenKeyAndSubmessages) { + verifyFormat("submessage: {\n" + " key: 'a'\n" + "}"); + verifyFormat("submessage {\n" + " key: 'a'\n" + "}"); + verifyFormat("submessage: <\n" + " key: 'a'\n" + ">"); + verifyFormat("submessage <\n" + " key: 'a'\n" + ">"); + verifyFormat("repeatedd: [\n" + " 'ey'\n" + "]"); + // "{" is going over the column limit. + verifyFormat( + "submessagee: {\n" + " key: 'a'\n" + "}"); +} + } // end namespace tooling } // end namespace clang Index: unittests/Format/FormatTestProto.cpp === --- unittests/Format/FormatTestProto.cpp +++ unittests/Format/FormatTestProto.cpp @@ -624,5 +624,34 @@ "}"); } +TEST_F(FormatTestProto, PreventBreaksBetweenKeyAndSubmessages) { + verifyFormat("option (MyProto.options) = {\n" + " submessage: {\n" + "key: 'a'\n" + " }\n" + "}"); + verifyFormat("option (MyProto.options) = {\n" + " submessage {\n" + "key: 'a'\n" + " }\n" + "}"); + verifyFormat("option (MyProto.options) = {\n" + " submessage: <\n" + "key: 'a'\n" + " >\n" + "}"); + verifyFormat("option (MyProto.options) = {\n" + " submessage <\n" + "key: 'a'\n" + " >\n" + "}"); + verifyFormat("option (MyProto.options) = {\n" + " repeatedd: [\n" + "'ey'\n" + " ]\n" + "}"); +} + + } // end namespace tooling } // end namespace clang Index: lib/Format/TokenAnnotator.cpp === --- lib/Format/TokenAnnotator.cpp +++ lib/Format/TokenAnnotator.cpp @@ -3101,10 +3101,39 @@ !Right.isOneOf(TT_CtorInitializerColon, TT_InlineASMColon)) return false; if (Left.is(tok::colon) && Left.isOneOf(TT_DictLiteral, TT_ObjCMethodExpr)) { -if ((Style.Language == FormatStyle::LK_Proto || - Style.Language == FormatStyle::LK_TextProto) && -!Style.AlwaysBreakBeforeMultilineStrings && Right.isStringLiteral()) - return false; +if (Style.Language == FormatStyle::LK_Proto || +Style.Language == FormatStyle::LK_TextProto) { + if (!Style.AlwaysBreakBeforeMultilineStrings && Right.isStringLiteral()) +return false; + // Prevent cases like: + // + // submessage: + // { key: valu } + // + // when the snippet does not fit into one line. + // Prefer: + // + // submessage: { + // key: valu + // } + // + // instead, even if it is longer by one line. + // + // Note that this allows allows the "{" to go over the column limit + // when the column limit is just between ":" and "{", but that does + // not happen too often and alternative formattings in this case are + // not much better. + // + // The code covers the cases: + // + // submessage: { ... } + // submessage: < ... > + // repeated: [ ... ] +
[PATCH] D48063: [clang-format] Discourage breaks in submessage entries, hard rule
sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: lib/Format/TokenAnnotator.cpp:3104 if (Left.is(tok::colon) && Left.isOneOf(TT_DictLiteral, TT_ObjCMethodExpr)) { if ((Style.Language == FormatStyle::LK_Proto || Style.Language == FormatStyle::LK_TextProto) && It's really hard to follow the boolean logic here. Can you break this up into multiple if statements with comments, or extract some named subexpressions or something? e.g. ``` if (proto or textproto) { if (bool isSubmessage = ...) return false; if (right is string && !break before multiline) return false; return true; } Repository: rC Clang https://reviews.llvm.org/D48063 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48063: [clang-format] Discourage breaks in submessage entries, hard rule
krasimir updated this revision to Diff 150926. krasimir added a comment. - Add tests Repository: rC Clang https://reviews.llvm.org/D48063 Files: lib/Format/TokenAnnotator.cpp unittests/Format/FormatTestProto.cpp unittests/Format/FormatTestTextProto.cpp Index: unittests/Format/FormatTestTextProto.cpp === --- unittests/Format/FormatTestTextProto.cpp +++ unittests/Format/FormatTestTextProto.cpp @@ -485,8 +485,15 @@ verifyFormat("keys: []"); verifyFormat("keys: [ 1 ]"); verifyFormat("keys: [ 'ala', 'bala' ]"); - verifyFormat("keys:\n" - "[ 'ala', 'bala', 'porto', 'kala', 'too', 'long', 'ng' ]"); + verifyFormat("keys: [\n" + " 'ala',\n" + " 'bala',\n" + " 'porto',\n" + " 'kala',\n" + " 'too',\n" + " 'long',\n" + " 'ng'\n" + "]"); verifyFormat("key: item\n" "keys: [\n" " 'ala',\n" @@ -670,5 +677,28 @@ "}"); } +TEST_F(FormatTestTextProto, PreventBreaksBetweenKeyAndSubmessages) { + verifyFormat("submessage: {\n" + " key: 'a'\n" + "}"); + verifyFormat("submessage {\n" + " key: 'a'\n" + "}"); + verifyFormat("submessage: <\n" + " key: 'a'\n" + ">"); + verifyFormat("submessage <\n" + " key: 'a'\n" + ">"); + verifyFormat("repeatedd: [\n" + " 'ey'\n" + "]"); + // "{" is going over the column limit. + verifyFormat( + "submessagee: {\n" + " key: 'a'\n" + "}"); +} + } // end namespace tooling } // end namespace clang Index: unittests/Format/FormatTestProto.cpp === --- unittests/Format/FormatTestProto.cpp +++ unittests/Format/FormatTestProto.cpp @@ -624,5 +624,34 @@ "}"); } +TEST_F(FormatTestProto, PreventBreaksBetweenKeyAndSubmessages) { + verifyFormat("option (MyProto.options) = {\n" + " submessage: {\n" + "key: 'a'\n" + " }\n" + "}"); + verifyFormat("option (MyProto.options) = {\n" + " submessage {\n" + "key: 'a'\n" + " }\n" + "}"); + verifyFormat("option (MyProto.options) = {\n" + " submessage: <\n" + "key: 'a'\n" + " >\n" + "}"); + verifyFormat("option (MyProto.options) = {\n" + " submessage <\n" + "key: 'a'\n" + " >\n" + "}"); + verifyFormat("option (MyProto.options) = {\n" + " repeatedd: [\n" + "'ey'\n" + " ]\n" + "}"); +} + + } // end namespace tooling } // end namespace clang Index: lib/Format/TokenAnnotator.cpp === --- lib/Format/TokenAnnotator.cpp +++ lib/Format/TokenAnnotator.cpp @@ -3103,7 +3103,35 @@ if (Left.is(tok::colon) && Left.isOneOf(TT_DictLiteral, TT_ObjCMethodExpr)) { if ((Style.Language == FormatStyle::LK_Proto || Style.Language == FormatStyle::LK_TextProto) && -!Style.AlwaysBreakBeforeMultilineStrings && Right.isStringLiteral()) +((!Style.AlwaysBreakBeforeMultilineStrings && + Right.isStringLiteral()) || + // Prevent cases like: + // + // submessage: + // { key: valu } + // + // when the snippet does not fit into one line. + // Prefer: + // + // submessage: { + // key: valu + // } + // + // instead, even if it is longer by one line. + // + // Note that this allows allows the "{" to go over the column limit + // when the column limit is just between ":" and "{", but that does + // not happen too often and alternative formattings in this case are + // not much better. + // + // The code covers the cases: + // + // submessage: { ... } + // submessage: < ... > + // repeated: [ ... ] + ((Right.is(tok::l_brace) || Right.is(tok::less)) && + Right.is(TT_DictLiteral)) || + Right.is(TT_ArrayInitializerLSquare))) return false;
[PATCH] D48063: [clang-format] Discourage breaks in submessage entries, hard rule
krasimir created this revision. Herald added a subscriber: cfe-commits. This is an alternative to https://reviews.llvm.org/D48034. Repository: rC Clang https://reviews.llvm.org/D48063 Files: lib/Format/TokenAnnotator.cpp unittests/Format/FormatTestTextProto.cpp Index: unittests/Format/FormatTestTextProto.cpp === --- unittests/Format/FormatTestTextProto.cpp +++ unittests/Format/FormatTestTextProto.cpp @@ -485,8 +485,15 @@ verifyFormat("keys: []"); verifyFormat("keys: [ 1 ]"); verifyFormat("keys: [ 'ala', 'bala' ]"); - verifyFormat("keys:\n" - "[ 'ala', 'bala', 'porto', 'kala', 'too', 'long', 'ng' ]"); + verifyFormat("keys: [\n" + " 'ala',\n" + " 'bala',\n" + " 'porto',\n" + " 'kala',\n" + " 'too',\n" + " 'long',\n" + " 'ng'\n" + "]"); verifyFormat("key: item\n" "keys: [\n" " 'ala',\n" @@ -670,5 +677,23 @@ "}"); } +TEST_F(FormatTestTextProto, TODO) { + verifyFormat("submessage: {\n" + " key: 'a'\n" + "}"); + verifyFormat("submessage {\n" + " key: 'a'\n" + "}"); + verifyFormat("submessage: <\n" + " key: 'a'\n" + ">"); + verifyFormat("submessage <\n" + " key: 'a'\n" + ">"); + verifyFormat("submessage: [\n" + " 'ey'\n" + "]"); +} + } // end namespace tooling } // end namespace clang Index: lib/Format/TokenAnnotator.cpp === --- lib/Format/TokenAnnotator.cpp +++ lib/Format/TokenAnnotator.cpp @@ -3101,10 +3101,16 @@ !Right.isOneOf(TT_CtorInitializerColon, TT_InlineASMColon)) return false; if (Left.is(tok::colon) && Left.isOneOf(TT_DictLiteral, TT_ObjCMethodExpr)) { -if ((Style.Language == FormatStyle::LK_Proto || - Style.Language == FormatStyle::LK_TextProto) && -!Style.AlwaysBreakBeforeMultilineStrings && Right.isStringLiteral()) - return false; +if (Style.Language == FormatStyle::LK_Proto || + Style.Language == FormatStyle::LK_TextProto) { + if (!Style.AlwaysBreakBeforeMultilineStrings && Right.isStringLiteral()) +return false; + if ((Right.is(tok::l_brace) || Right.is(tok::less)) && + Right.is(TT_DictLiteral)) +return false; + if (Right.is(TT_ArrayInitializerLSquare)) +return false; +} return true; } if (Right.is(tok::r_square) && Right.MatchingParen && Index: unittests/Format/FormatTestTextProto.cpp === --- unittests/Format/FormatTestTextProto.cpp +++ unittests/Format/FormatTestTextProto.cpp @@ -485,8 +485,15 @@ verifyFormat("keys: []"); verifyFormat("keys: [ 1 ]"); verifyFormat("keys: [ 'ala', 'bala' ]"); - verifyFormat("keys:\n" - "[ 'ala', 'bala', 'porto', 'kala', 'too', 'long', 'ng' ]"); + verifyFormat("keys: [\n" + " 'ala',\n" + " 'bala',\n" + " 'porto',\n" + " 'kala',\n" + " 'too',\n" + " 'long',\n" + " 'ng'\n" + "]"); verifyFormat("key: item\n" "keys: [\n" " 'ala',\n" @@ -670,5 +677,23 @@ "}"); } +TEST_F(FormatTestTextProto, TODO) { + verifyFormat("submessage: {\n" + " key: 'a'\n" + "}"); + verifyFormat("submessage {\n" + " key: 'a'\n" + "}"); + verifyFormat("submessage: <\n" + " key: 'a'\n" + ">"); + verifyFormat("submessage <\n" + " key: 'a'\n" + ">"); + verifyFormat("submessage: [\n" + " 'ey'\n" + "]"); +} + } // end namespace tooling } // end namespace clang Index: lib/Format/TokenAnnotator.cpp === --- lib/Format/TokenAnnotator.cpp +++ lib/Format/TokenAnnotator.cpp @@ -3101,10 +3101,16 @@ !Right.isOneOf(TT_CtorInitializerColon, TT_InlineASMColon)) return false; if (Left.is(tok::colon) && Left.isOneOf(TT_DictLiteral, TT_ObjCMethodExpr)) { -if ((Style.Language == FormatStyle::LK_Proto || - Style.Language == FormatStyle::LK_TextProto) && -