r227427 - clang-format: Fix crasher caused by not properly setting dry-run.

[Daniel Jasper]

Not necessarily. I don't care what the output in those particular cases is.
I want them to not crash, not access uninitialized memory, etc.

clang-format has certain logic that makes it do good modification to
invalid code, which is highly useful. We aren't copping out of formatting
code just because it is broken. However, in these particular cases, I don't
care. Yes, I could try to come up with a saner test that both crashes
clang-format as well as contains input where I care about the formatting,
but I don't think that that would be effort well-spent.

On Thu, Jan 29, 2015 at 8:52 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Thu, Jan 29, 2015 at 2:47 AM, Daniel Jasper <djasper at google.com> wrote:
>
>> Author: djasper
>> Date: Thu Jan 29 04:47:14 2015
>> New Revision: 227427
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=227427&view=rev
>> Log:
>> clang-format: Fix crasher caused by not properly setting dry-run.
>>
>> Modified:
>>     cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
>>     cfe/trunk/lib/Format/WhitespaceManager.cpp
>>     cfe/trunk/unittests/Format/FormatTest.cpp
>>
>> Modified: cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp?rev=227427&r1=227426&r2=227427&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp (original)
>> +++ cfe/trunk/lib/Format/UnwrappedLineFormatter.cpp Thu Jan 29 04:47:14
>> 2015
>> @@ -409,7 +409,7 @@ UnwrappedLineFormatter::format(const Sma
>>            TheLine.Type == LT_ImportStatement) {
>>          LineState State = Indenter->getInitialState(Indent, &TheLine,
>> DryRun);
>>          while (State.NextToken) {
>> -          formatChildren(State, /*Newline=*/false, /*DryRun=*/false,
>> Penalty);
>> +          formatChildren(State, /*Newline=*/false, DryRun, Penalty);
>>            Indenter->addTokenToState(State, /*Newline=*/false, DryRun);
>>          }
>>        } else if (Style.ColumnLimit == 0) {
>> @@ -657,8 +657,8 @@ void UnwrappedLineFormatter::addNextStat
>>
>>  bool UnwrappedLineFormatter::formatChildren(LineState &State, bool
>> NewLine,
>>                                              bool DryRun, unsigned
>> &Penalty) {
>> -  FormatToken &Previous = *State.NextToken->Previous;
>>    const FormatToken *LBrace = State.NextToken->getPreviousNonComment();
>> +  FormatToken &Previous = *State.NextToken->Previous;
>>    if (!LBrace || LBrace->isNot(tok::l_brace) || LBrace->BlockKind !=
>> BK_Block ||
>>        Previous.Children.size() == 0)
>>      // The previous token does not open a block. Nothing to do. We don't
>>
>> Modified: cfe/trunk/lib/Format/WhitespaceManager.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/WhitespaceManager.cpp?rev=227427&r1=227426&r2=227427&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/lib/Format/WhitespaceManager.cpp (original)
>> +++ cfe/trunk/lib/Format/WhitespaceManager.cpp Thu Jan 29 04:47:14 2015
>> @@ -264,6 +264,11 @@ void WhitespaceManager::alignEscapedNewl
>>  void WhitespaceManager::generateChanges() {
>>    for (unsigned i = 0, e = Changes.size(); i != e; ++i) {
>>      const Change &C = Changes[i];
>> +    if (i > 0) {
>> +      assert(Changes[i - 1].OriginalWhitespaceRange.getBegin() !=
>> +                 C.OriginalWhitespaceRange.getBegin() &&
>> +             "Generating two replacements for the same location");
>> +    }
>>      if (C.CreateReplacement) {
>>        std::string ReplacementText = C.PreviousLinePostfix;
>>        if (C.ContinuesPPDirective)
>>
>> Modified: cfe/trunk/unittests/Format/FormatTest.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=227427&r1=227426&r2=227427&view=diff
>>
>> ==============================================================================
>> --- cfe/trunk/unittests/Format/FormatTest.cpp (original)
>> +++ cfe/trunk/unittests/Format/FormatTest.cpp Thu Jan 29 04:47:14 2015
>> @@ -3081,6 +3081,8 @@ TEST_F(FormatTest, LayoutNestedBlocks) {
>>                 "  if (aaaaaaaaaaaaaaaaaaaaaaaa) return;\n"
>>                 "}, a);",
>>                 Style);
>> +
>> +  verifyNoCrash("^{v^{a}}");
>>
>
> "not crashing" is a pretty low bar for a test. Is there some behavior that
> should be expected? (I guess most of these crashers are crash-on-invalid
> and the correct behavior is to not modify the text?)
>
>
>>  }
>>
>>  TEST_F(FormatTest, IndividualStatementsOfNestedBlocks) {
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150130/16aeaf6c/attachment.html>