Page MenuHomePhabricator

[clang-format] Handle attributes before case label.
ClosedPublic

Authored by curdeius on Mar 11 2022, 1:15 AM.

Diff Detail

Event Timeline

curdeius created this revision.Mar 11 2022, 1:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2022, 1:15 AM
curdeius requested review of this revision.Mar 11 2022, 1:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2022, 1:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
This revision is now accepted and ready to land.Mar 11 2022, 11:53 AM
owenpan accepted this revision.Mar 11 2022, 2:44 PM
owenpan added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
578

Can we remove this empty line?

clang/lib/Format/UnwrappedLineParser.h
127–128

Can we move them 1 line up so that the order of the declarations here will be the same as that of the definitions in the .cpp file?

MyDeveloperDay accepted this revision.Mar 11 2022, 11:47 PM
curdeius added inline comments.Mar 12 2022, 12:14 AM
clang/lib/Format/UnwrappedLineParser.cpp
578

Will do.

clang/lib/Format/UnwrappedLineParser.h
127–128

Will do.

This revision was automatically updated to reflect the committed changes.
curdeius marked 2 inline comments as done.
jgorbe added a subscriber: jgorbe.Mar 21 2022, 2:34 PM

Hi,

We've observed that this patch introduces infinite loops in some cases. Here's a reduced test case:

export class Foo extends Bar {
  get case(): Case {
    return (
        (this.Bar$has('case')) ? (this.Bar$get('case')) :
                                 (this.case = new Case()));
  }
}

Saving this as /tmp/test.ts and running clang-format /tmp/test.ts loops indefinitely with this patch (I stopped it after 1m27s) and finishes instantly (0.12s) after reverting the patch locally. I'm going to go ahead and push a revert. Please let me know if you need help reproducing the problem.

clang/lib/Format/UnwrappedLineParser.cpp
1826

Here's the loop. In the switch before that worked, since it was not in a loop. Here we need to do something. (nextToken()?)

Hi,

We've observed that this patch introduces infinite loops in some cases. Here's a reduced test case:

export class Foo extends Bar {
  get case(): Case {
    return (
        (this.Bar$has('case')) ? (this.Bar$get('case')) :
                                 (this.case = new Case()));
  }
}

Saving this as /tmp/test.ts and running clang-format /tmp/test.ts loops indefinitely with this patch (I stopped it after 1m27s) and finishes instantly (0.12s) after reverting the patch locally. I'm going to go ahead and push a revert. Please let me know if you need help reproducing the problem.

Thanks for the reproducer.

clang/lib/Format/UnwrappedLineParser.cpp
1826

Yes, it seems so. I'll add a test case and fix it.