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