Page MenuHomePhabricator

[clang-format] Adjust braced list detection
Needs RevisionPublic

Authored by cpplearner on Thu, Nov 25, 4:43 AM.

Details

Summary

This avoids mishandling nested compound statements that are followed by another compound statement.

Fixes https://llvm.org/PR38314 and https://llvm.org/PR48305.

Diff Detail

Event Timeline

cpplearner requested review of this revision.Thu, Nov 25, 4:43 AM
cpplearner created this revision.

Thanks for working on this.
First of all, the test seems ok for https://llvm.org/PR48305, but I'd like to see a test case for https://llvm.org/PR38314 (since you wrote in the description that this bug gets fixed as well).
And of course, we need to make the failing tests pass! :)

curdeius edited the summary of this revision. (Show Details)Thu, Nov 25, 6:47 AM
HazardyKnusperkeks removed a reviewer: Restricted Project.Thu, Nov 25, 7:40 AM
HazardyKnusperkeks added a project: Restricted Project.
MyDeveloperDay set the repository for this revision to rG LLVM Github Monorepo.
MyDeveloperDay added a project: Restricted Project.
MyDeveloperDay added a subscriber: cfe-commits.

FormatsBracedListsInColumnLayout is actually failing in its own test... it now produces

void foo() {
  { // asdf
    { int a; }
  }
  {
    { int b; }
  }
}

rather than previously

void foo() {
  {// asdf
   {int a;
}
}
{
  { int b; }
}
}

The other failure is

-class C extends {} {}
+class C extends {}
+{}

I'm slightly struggling to work out in both cases if its the test that is wrong @cpplearner what do you think?

For your test I think its your test that is wrong, looking at the simpler versions (using clang-format-12)

void foo() {
  {
    { int a; }
  }
}

void foo() {
  { int a; }
}
cpplearner updated this revision to Diff 389822.EditedThu, Nov 25, 9:23 AM

Fixed tests.

Since in JavaScript, the thing after extends must be an expression, and the thing after implements must be an object type, I decided to parse both by directly calling parseBracedList, instead of going through tryToParseBracedList.

MyDeveloperDay accepted this revision.Thu, Nov 25, 9:38 AM

I ran the examples from https://bugs.llvm.org/show_bug.cgi?id=38314 and its so much better than the previous state!

Thanks for this patch, LGTM

namespace n {
void foo() {
    {
        {
            {
                statement();
                if (false) {
                }
            }
        }
        {}
    }
}
}  // namespace n

void foo() {
    {
        {
            statement();
            if (false) {
            }
        }
    }
    {}
}  // namespace n
This revision is now accepted and ready to land.Thu, Nov 25, 9:38 AM
HazardyKnusperkeks requested changes to this revision.Thu, Nov 25, 1:03 PM

That will be a pain in the ass to merge with my current changes, but looks good.

First of all, the test seems ok for https://llvm.org/PR48305, but I'd like to see a test case for https://llvm.org/PR38314 (since you wrote in the description that this bug gets fixed as well).

But I second that.

This revision now requires changes to proceed.Thu, Nov 25, 1:03 PM