<div dir="ltr"><div class="gmail_default" style>On Tue, Jan 8, 2013 at 11:06 PM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank" class="cremed">djasper@google.com</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: djasper<br>
Date: Wed Jan 9 01:06:56 2013<br>
New Revision: 171957<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=171957&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=171957&view=rev</a><br>
Log:<br>
Improve formatting of conditional operators.<br>
<br>
This addresses <a href="http://llvm.org/PR14864" target="_blank" class="cremed">llvm.org/PR14864</a>.<br>
<br>
We used to completely mess this up and now format as:<br>
Diag(NewFD->getLocation(),<br>
getLangOpts().MicrosoftExt ? diag::ext_function_specialization_in_class :<br>
diag::err_function_specialization_in_class)<br>
<< NewFD->getDeclName();<br></blockquote><div><br></div><div style><breaks out the bikeshed paint></div><div style><br></div><div style>There is a reasonably common pattern in Clang-land at least that I find significantly more readable:</div>
<div style><br></div><div style><font face="courier new, monospace">Diag(NewFD->getLocation(),<br> getLangOpts().MicrosoftExt ? diag::ext_function_specialization_in_class</font></div><div style><font face="courier new, monospace"> : diag::err_function_specialization_in_class)<br>
<< NewFD->getDeclName();<br></font></div><div><br></div><div style>Thoughts?</div><div style><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Modified:<br>
cfe/trunk/lib/Format/Format.cpp<br>
cfe/trunk/unittests/Format/FormatTest.cpp<br>
<br>
Modified: cfe/trunk/lib/Format/Format.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=171957&r1=171956&r2=171957&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=171957&r1=171956&r2=171957&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Format/Format.cpp (original)<br>
+++ cfe/trunk/lib/Format/Format.cpp Wed Jan 9 01:06:56 2013<br>
@@ -269,30 +269,32 @@<br>
/// If \p DryRun is \c false, also creates and stores the required<br>
/// \c Replacement.<br>
void addTokenToState(bool Newline, bool DryRun, IndentState &State) {<br>
- const FormatToken &Current = State.NextToken->FormatTok;<br>
- const FormatToken &Previous = State.NextToken->Parent->FormatTok;<br>
+ const AnnotatedToken &Current = *State.NextToken;<br>
+ const AnnotatedToken &Previous = *State.NextToken->Parent;<br>
unsigned ParenLevel = State.Indent.size() - 1;<br>
<br>
if (Newline) {<br>
unsigned WhitespaceStartColumn = State.Column;<br>
- if (<a href="http://Previous.Tok.is" target="_blank" class="cremed">Previous.Tok.is</a>(tok::l_brace)) {<br>
+ if (Previous.is(tok::l_brace)) {<br>
// FIXME: This does not work with nested static initializers.<br>
// Implement a better handling for static initializers and similar<br>
// constructs.<br>
State.Column = Line.Level * 2 + 2;<br>
- } else if (<a href="http://Current.Tok.is" target="_blank" class="cremed">Current.Tok.is</a>(tok::string_literal) &&<br>
- <a href="http://Previous.Tok.is" target="_blank" class="cremed">Previous.Tok.is</a>(tok::string_literal)) {<br>
- State.Column = State.Column - Previous.TokenLength;<br>
- } else if (<a href="http://Current.Tok.is" target="_blank" class="cremed">Current.Tok.is</a>(tok::lessless) &&<br>
+ } else if (Current.is(tok::string_literal) &&<br>
+ Previous.is(tok::string_literal)) {<br>
+ State.Column = State.Column - Previous.FormatTok.TokenLength;<br>
+ } else if (Current.is(tok::lessless) &&<br>
State.FirstLessLess[ParenLevel] != 0) {<br>
State.Column = State.FirstLessLess[ParenLevel];<br>
} else if (ParenLevel != 0 &&<br>
- (<a href="http://Previous.Tok.is" target="_blank" class="cremed">Previous.Tok.is</a>(tok::equal) || <a href="http://Current.Tok.is" target="_blank" class="cremed">Current.Tok.is</a>(tok::arrow) ||<br>
- <a href="http://Current.Tok.is" target="_blank" class="cremed">Current.Tok.is</a>(tok::period))) {<br>
- // Indent and extra 4 spaces after '=' as it continues an expression.<br>
- // Don't do that on the top level, as we already indent 4 there.<br>
+ (Previous.is(tok::equal) || Current.is(tok::arrow) ||<br>
+ Current.is(tok::period) || Previous.is(tok::question) ||<br>
+ Previous.Type == TT_ConditionalExpr)) {<br>
+ // Indent and extra 4 spaces after if we know the current expression is<br>
+ // continued. Don't do that on the top level, as we already indent 4<br>
+ // there.<br>
State.Column = State.Indent[ParenLevel] + 4;<br>
- } else if (RootToken.is(tok::kw_for) && <a href="http://Previous.Tok.is" target="_blank" class="cremed">Previous.Tok.is</a>(tok::comma)) {<br>
+ } else if (RootToken.is(tok::kw_for) && Previous.is(tok::comma)) {<br>
State.Column = State.ForLoopVariablePos;<br>
} else if (State.NextToken->Parent->ClosesTemplateDeclaration) {<br>
State.Column = State.Indent[ParenLevel] - 4;<br>
@@ -303,23 +305,24 @@<br>
State.StartOfLineLevel = ParenLevel + 1;<br>
<br>
if (RootToken.is(tok::kw_for))<br>
- State.LineContainsContinuedForLoopSection =<br>
- Previous.Tok.isNot(tok::semi);<br>
+ State.LineContainsContinuedForLoopSection = Previous.isNot(tok::semi);<br>
<br>
if (!DryRun) {<br>
if (!Line.InPPDirective)<br>
- replaceWhitespace(Current, 1, State.Column);<br>
+ replaceWhitespace(Current.FormatTok, 1, State.Column);<br>
else<br>
- replacePPWhitespace(Current, 1, State.Column, WhitespaceStartColumn);<br>
+ replacePPWhitespace(Current.FormatTok, 1, State.Column,<br>
+ WhitespaceStartColumn);<br>
}<br>
<br>
State.LastSpace[ParenLevel] = State.Indent[ParenLevel];<br>
- if (<a href="http://Current.Tok.is" target="_blank" class="cremed">Current.Tok.is</a>(tok::colon) && CurrentLineType != LT_ObjCMethodDecl &&<br>
+ if (Current.is(tok::colon) && CurrentLineType != LT_ObjCMethodDecl &&<br>
State.NextToken->Type != TT_ConditionalExpr)<br>
State.Indent[ParenLevel] += 2;<br>
} else {<br>
- if (<a href="http://Current.Tok.is" target="_blank" class="cremed">Current.Tok.is</a>(tok::equal) && RootToken.is(tok::kw_for))<br>
- State.ForLoopVariablePos = State.Column - Previous.TokenLength;<br>
+ if (Current.is(tok::equal) && RootToken.is(tok::kw_for))<br>
+ State.ForLoopVariablePos = State.Column -<br>
+ Previous.FormatTok.TokenLength;<br>
<br>
unsigned Spaces = State.NextToken->SpaceRequiredBefore ? 1 : 0;<br>
if (State.NextToken->Type == TT_LineComment)<br>
@@ -330,9 +333,9 @@<br>
<br>
if (RootToken.isNot(tok::kw_for) &&<br>
(getPrecedence(Previous) == prec::Assignment ||<br>
- <a href="http://Previous.Tok.is" target="_blank" class="cremed">Previous.Tok.is</a>(tok::kw_return)))<br>
+ Previous.is(tok::kw_return)))<br>
State.Indent[ParenLevel] = State.Column + Spaces;<br>
- if (<a href="http://Previous.Tok.is" target="_blank" class="cremed">Previous.Tok.is</a>(tok::l_paren) ||<br>
+ if (Previous.is(tok::l_paren) ||<br>
State.NextToken->Parent->Type == TT_TemplateOpener)<br>
State.Indent[ParenLevel] = State.Column;<br>
<br>
@@ -398,6 +401,8 @@<br>
if (Left.is(tok::l_paren))<br>
return 20;<br>
<br>
+ if (Left.is(tok::question) || Left.Type == TT_ConditionalExpr)<br>
+ return prec::Assignment;<br>
prec::Level Level = getPrecedence(Left);<br>
<br>
// Breaking after an assignment leads to a bad result as the two sides of<br>
@@ -484,10 +489,11 @@<br>
<br>
/// \brief Replaces the whitespace in front of \p Tok. Only call once for<br>
/// each \c FormatToken.<br>
- void replaceWhitespace(const FormatToken &Tok, unsigned NewLines,<br>
+ void replaceWhitespace(const AnnotatedToken &Tok, unsigned NewLines,<br>
unsigned Spaces) {<br>
Replaces.insert(tooling::Replacement(<br>
- SourceMgr, Tok.WhiteSpaceStart, Tok.WhiteSpaceLength,<br>
+ SourceMgr, Tok.FormatTok.WhiteSpaceStart,<br>
+ Tok.FormatTok.WhiteSpaceLength,<br>
std::string(NewLines, '\n') + std::string(Spaces, ' ')));<br>
}<br>
<br>
@@ -496,7 +502,7 @@<br>
///<br>
/// This function and \c replaceWhitespace have the same behavior if<br>
/// \c Newlines == 0.<br>
- void replacePPWhitespace(const FormatToken &Tok, unsigned NewLines,<br>
+ void replacePPWhitespace(const AnnotatedToken &Tok, unsigned NewLines,<br>
unsigned Spaces, unsigned WhitespaceStartColumn) {<br>
std::string NewLineText;<br>
if (NewLines > 0) {<br>
@@ -508,9 +514,10 @@<br>
Offset = 0;<br>
}<br>
}<br>
- Replaces.insert(tooling::Replacement(SourceMgr, Tok.WhiteSpaceStart,<br>
- Tok.WhiteSpaceLength, NewLineText +<br>
- std::string(Spaces, ' ')));<br>
+ Replaces.insert(<br>
+ tooling::Replacement(SourceMgr, Tok.FormatTok.WhiteSpaceStart,<br>
+ Tok.FormatTok.WhiteSpaceLength,<br>
+ NewLineText + std::string(Spaces, ' ')));<br>
}<br>
<br>
/// \brief Add a new line and the required indent before the first Token<br>
@@ -1061,7 +1068,8 @@<br>
Left.is(tok::comma) || Right.is(tok::lessless) ||<br>
Right.is(tok::arrow) || Right.is(tok::period) ||<br>
Right.is(tok::colon) || Left.is(tok::semi) ||<br>
- Left.is(tok::l_brace) ||<br>
+ Left.is(tok::l_brace) || Left.is(tok::question) ||<br>
+ Left.Type == TT_ConditionalExpr ||<br>
(Left.is(tok::l_paren) && !Right.is(tok::r_paren));<br>
}<br>
<br>
<br>
Modified: cfe/trunk/unittests/Format/FormatTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=171957&r1=171956&r2=171957&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=171957&r1=171956&r2=171957&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/unittests/Format/FormatTest.cpp (original)<br>
+++ cfe/trunk/unittests/Format/FormatTest.cpp Wed Jan 9 01:06:56 2013<br>
@@ -759,6 +759,15 @@<br>
" aaaaaaaaaaaaaaaaaaaaaaaaa);");<br>
}<br>
<br>
+TEST_F(FormatTest, BreaksConditionalExpressions) {<br>
+ verifyFormat(<br>
+ "aaaa(aaaaaaaaaaaaaaaaaaaa,\n"<br>
+ " aaaaaaaaaaaaaaaaaaaaaaaaaa ? aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa :\n"<br>
+ " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);");<br>
+ verifyFormat("aaaa(aaaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaaaaaa ?\n"<br>
+ " aaaaaaaaaaaaaaaaaaaaaaa : aaaaaaaaaaaaaaaaaaaaa);");<br>
+}<br>
+<br>
TEST_F(FormatTest, AlignsStringLiterals) {<br>
verifyFormat("loooooooooooooooooooooooooongFunction(\"short literal \"\n"<br>
" \"short literal\");");<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div></div>