<div dir="ltr">Thanks for catching this. I think the only bug was the ">" instead of "!=". However, cleaning this up makes a lot of sense. Submitted cleanup in r175500.</div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Tue, Feb 19, 2013 at 2:30 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I'm not 100% certain that this is the commit which caused the breakage<br>
I am seeing, but operator< causes assertions to fail in MSVC's<br>
predicate debuggers. Long story short, for some LineStates,<br>
!pred(left, right) fails, and pred(right, left) succeeds which causes<br>
an assertion.<br>
<br>
One thing which struck me as potentially odd was that the code was<br>
using Other > this in some places, and Other < this in other places.<br>
When I flip the logic to be consistent, all of the assertions quieted<br>
down. I've attached a patch showing the changes I've made, but I'd<br>
like some oversight to make sure I've not mucked things up.<br>
<br>
Thanks!<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Mon, Feb 18, 2013 at 6:05 AM, Daniel Jasper <<a href="mailto:djasper@google.com">djasper@google.com</a>> wrote:<br>
> Author: djasper<br>
> Date: Mon Feb 18 05:05:07 2013<br>
> New Revision: 175432<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=175432&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=175432&view=rev</a><br>
> Log:<br>
> Prevent line breaks that make stuff hard to read.<br>
><br>
> Before:<br>
> aaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa).aaaaaaaaaaaaaaaaa(<br>
> aaaaaaaaaaaaaaaaaaaaa).aaaaaaaaaaaaaaaaaaa(<br>
> aaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);<br>
><br>
> After:<br>
> aaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa)<br>
> .aaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa)<br>
> .aaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaa,<br>
> aaaaaaaaaaaaaaaaaaa,<br>
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);<br>
><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=175432&r1=175431&r2=175432&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=175432&r1=175431&r2=175432&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Format/Format.cpp (original)<br>
> +++ cfe/trunk/lib/Format/Format.cpp Mon Feb 18 05:05:07 2013<br>
> @@ -251,6 +251,7 @@ public:<br>
> State.VariablePos = 0;<br>
> State.LineContainsContinuedForLoopSection = false;<br>
> State.ParenLevel = 0;<br>
> + State.StartOfLineLevel = State.ParenLevel;<br>
><br>
> DEBUG({<br>
> DebugTokenState(*State.NextToken);<br>
> @@ -384,6 +385,9 @@ private:<br>
> /// \brief The level of nesting inside (), [], <> and {}.<br>
> unsigned ParenLevel;<br>
><br>
> + /// \brief The \c ParenLevel at the start of this line.<br>
> + unsigned StartOfLineLevel;<br>
> +<br>
> /// \brief A stack keeping track of properties applying to parenthesis<br>
> /// levels.<br>
> std::vector<ParenState> Stack;<br>
> @@ -401,6 +405,8 @@ private:<br>
> return LineContainsContinuedForLoopSection;<br>
> if (Other.ParenLevel != ParenLevel)<br>
> return Other.ParenLevel < ParenLevel;<br>
> + if (Other.StartOfLineLevel < StartOfLineLevel)<br>
> + return Other.StartOfLineLevel < StartOfLineLevel;<br>
> return Other.Stack < Stack;<br>
> }<br>
> };<br>
> @@ -489,6 +495,7 @@ private:<br>
> }<br>
><br>
> State.Stack.back().LastSpace = State.Column;<br>
> + State.StartOfLineLevel = State.ParenLevel;<br>
> if (Current.is(tok::colon) && Current.Type != TT_ConditionalExpr)<br>
> State.Stack.back().Indent += 2;<br>
> } else {<br>
> @@ -772,6 +779,14 @@ private:<br>
> !(State.NextToken->is(tok::r_brace) &&<br>
> State.Stack.back().BreakBeforeClosingBrace))<br>
> return false;<br>
> + // This prevents breaks like:<br>
> + // ...<br>
> + // SomeParameter, OtherParameter).DoSomething(<br>
> + // ...<br>
> + // As they hide "DoSomething" and generally bad for readability.<br>
> + if (State.NextToken->Parent->is(tok::l_paren) &&<br>
> + State.ParenLevel <= State.StartOfLineLevel)<br>
> + return false;<br>
> // Trying to insert a parameter on a new line if there are already more than<br>
> // one parameter on the current line is bin packing.<br>
> if (State.Stack.back().HasMultiParameterLine &&<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=175432&r1=175431&r2=175432&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=175432&r1=175431&r2=175432&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)<br>
> +++ cfe/trunk/unittests/Format/FormatTest.cpp Mon Feb 18 05:05:07 2013<br>
> @@ -1457,6 +1457,13 @@ TEST_F(FormatTest, WrapsAtFunctionCallsI<br>
> verifyFormat("SomeMap[std::pair(aaaaaaaaaaaa, bbbbbbbbbbbbbbb)]\n"<br>
> " .insert(ccccccccccccccccccccccc);");<br>
><br>
> + verifyGoogleFormat(<br>
> + "aaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa)\n"<br>
> + " .aaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa)\n"<br>
> + " .aaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaa,\n"<br>
> + " aaaaaaaaaaaaaaaaaaa,\n"<br>
> + " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);");<br>
> +<br>
> // Here, it is not necessary to wrap at "." or "->".<br>
> verifyFormat("if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaa) ||\n"<br>
> " aaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n}");<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
<br>
</div></div><div class="HOEnZb"><div class="h5">On Mon, Feb 18, 2013 at 6:05 AM, Daniel Jasper <<a href="mailto:djasper@google.com">djasper@google.com</a>> wrote:<br>
> Author: djasper<br>
> Date: Mon Feb 18 05:05:07 2013<br>
> New Revision: 175432<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=175432&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=175432&view=rev</a><br>
> Log:<br>
> Prevent line breaks that make stuff hard to read.<br>
><br>
> Before:<br>
> aaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa).aaaaaaaaaaaaaaaaa(<br>
> aaaaaaaaaaaaaaaaaaaaa).aaaaaaaaaaaaaaaaaaa(<br>
> aaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaa, aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);<br>
><br>
> After:<br>
> aaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa)<br>
> .aaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa)<br>
> .aaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaa,<br>
> aaaaaaaaaaaaaaaaaaa,<br>
> aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);<br>
><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=175432&r1=175431&r2=175432&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/Format.cpp?rev=175432&r1=175431&r2=175432&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Format/Format.cpp (original)<br>
> +++ cfe/trunk/lib/Format/Format.cpp Mon Feb 18 05:05:07 2013<br>
> @@ -251,6 +251,7 @@ public:<br>
> State.VariablePos = 0;<br>
> State.LineContainsContinuedForLoopSection = false;<br>
> State.ParenLevel = 0;<br>
> + State.StartOfLineLevel = State.ParenLevel;<br>
><br>
> DEBUG({<br>
> DebugTokenState(*State.NextToken);<br>
> @@ -384,6 +385,9 @@ private:<br>
> /// \brief The level of nesting inside (), [], <> and {}.<br>
> unsigned ParenLevel;<br>
><br>
> + /// \brief The \c ParenLevel at the start of this line.<br>
> + unsigned StartOfLineLevel;<br>
> +<br>
> /// \brief A stack keeping track of properties applying to parenthesis<br>
> /// levels.<br>
> std::vector<ParenState> Stack;<br>
> @@ -401,6 +405,8 @@ private:<br>
> return LineContainsContinuedForLoopSection;<br>
> if (Other.ParenLevel != ParenLevel)<br>
> return Other.ParenLevel < ParenLevel;<br>
> + if (Other.StartOfLineLevel < StartOfLineLevel)<br>
> + return Other.StartOfLineLevel < StartOfLineLevel;<br>
> return Other.Stack < Stack;<br>
> }<br>
> };<br>
> @@ -489,6 +495,7 @@ private:<br>
> }<br>
><br>
> State.Stack.back().LastSpace = State.Column;<br>
> + State.StartOfLineLevel = State.ParenLevel;<br>
> if (Current.is(tok::colon) && Current.Type != TT_ConditionalExpr)<br>
> State.Stack.back().Indent += 2;<br>
> } else {<br>
> @@ -772,6 +779,14 @@ private:<br>
> !(State.NextToken->is(tok::r_brace) &&<br>
> State.Stack.back().BreakBeforeClosingBrace))<br>
> return false;<br>
> + // This prevents breaks like:<br>
> + // ...<br>
> + // SomeParameter, OtherParameter).DoSomething(<br>
> + // ...<br>
> + // As they hide "DoSomething" and generally bad for readability.<br>
> + if (State.NextToken->Parent->is(tok::l_paren) &&<br>
> + State.ParenLevel <= State.StartOfLineLevel)<br>
> + return false;<br>
> // Trying to insert a parameter on a new line if there are already more than<br>
> // one parameter on the current line is bin packing.<br>
> if (State.Stack.back().HasMultiParameterLine &&<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=175432&r1=175431&r2=175432&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=175432&r1=175431&r2=175432&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)<br>
> +++ cfe/trunk/unittests/Format/FormatTest.cpp Mon Feb 18 05:05:07 2013<br>
> @@ -1457,6 +1457,13 @@ TEST_F(FormatTest, WrapsAtFunctionCallsI<br>
> verifyFormat("SomeMap[std::pair(aaaaaaaaaaaa, bbbbbbbbbbbbbbb)]\n"<br>
> " .insert(ccccccccccccccccccccccc);");<br>
><br>
> + verifyGoogleFormat(<br>
> + "aaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa)\n"<br>
> + " .aaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaa)\n"<br>
> + " .aaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaa,\n"<br>
> + " aaaaaaaaaaaaaaaaaaa,\n"<br>
> + " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa);");<br>
> +<br>
> // Here, it is not necessary to wrap at "." or "->".<br>
> verifyFormat("if (aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaa) ||\n"<br>
> " aaaa.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) {\n}");<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div>