Page MenuHomePhabricator

[clang-format] PR48594 BraceWrapping: SplitEmptyRecord ignored for templates
ClosedPublic

Authored by MyDeveloperDay on Dec 27 2020, 7:29 AM.

Details

Summary

https://bugs.llvm.org/show_bug.cgi?id=48594

Empty or small templates were not being treated the same way as small classes especially when SplitEmptyRecord was set to true

This revision aims to help this by identifying a case when we should try not to merge the lines together

BraceWrapping:
  AfterStruct: true
  SplitEmptyRecord: true
BreakBeforeBraces: Custom

Input and expected formatted result:

C++
template <class> struct rep
{
};

Actual output:

C++
template <class> struct rep
{};

Diff Detail

Event Timeline

MyDeveloperDay requested review of this revision.Dec 27 2020, 7:29 AM
MyDeveloperDay created this revision.
MyDeveloperDay added a reviewer: JohelEGP.
JohelEGP accepted this revision.Dec 27 2020, 11:59 AM

Thank you.

This revision is now accepted and ready to land.Dec 27 2020, 11:59 AM
HazardyKnusperkeks added inline comments.
clang/lib/Format/UnwrappedLineFormatter.cpp
252

Why is this not just also in the previous if?
I see that the condition is a bit different, but what would happen if the condition is SplitEmptyRecord && !EmptyBlock?

I have another case:
.clang-format:

BraceWrapping:
  AfterStruct: true
  SplitEmptyRecord: true
BreakBeforeBraces: Custom

Input and expected formatted output:

template <std::regular T> struct rep<std::complex<T>>
{
  using type = T;
};

Actual formatted output:

template <std::regular T> struct rep<std::complex<T>>
{ using type = T; };
JohelEGP requested changes to this revision.Dec 27 2020, 5:07 PM
This revision now requires changes to proceed.Dec 27 2020, 5:07 PM

Addressing additional usecase found issues in C# tests too

MyDeveloperDay added inline comments.Dec 28 2020, 7:29 AM
clang/lib/Format/UnwrappedLineFormatter.cpp
252

non empty blocks are handled by the code below.

The reason the condition is different is to do with the fact that the merging is required to ensure concepts are not overly broken apart, this condition purely detects where we want to ensure we split the empty block.

JohelEGP accepted this revision.Dec 28 2020, 1:27 PM
This revision is now accepted and ready to land.Dec 28 2020, 1:27 PM
curdeius requested changes to this revision.Dec 29 2020, 2:56 PM
curdeius added inline comments.
clang/lib/Format/UnwrappedLineFormatter.cpp
364

Could you please add a comment about this block of code?
Something's like "Don't merge blocks when SplitEmptyRecords is enabled." or whatever you judge more useful/correct.

370

You might want to factor out the check of Previous from this and the next if.

375

You should check the type of Previous before computing PreviousPrevious to avoid the latter when unnecessary.

clang/unittests/Format/FormatTest.cpp
9952

What about testing specializations with empty body?

This revision now requires changes to proceed.Dec 29 2020, 2:56 PM
MyDeveloperDay marked 5 inline comments as done.

Address review comments

curdeius accepted this revision.Dec 30 2020, 2:56 AM

LGTM. Thank you!

This revision is now accepted and ready to land.Dec 30 2020, 2:56 AM

Hi. Up until this point, we've noticed that this patch can produce different formatted files depending on amount of whitespace. For example, given (A):

#include <stdint.h>
namespace fuzzing {}

clang-format with this patch would produced (B):

#include <stdint.h>
namespace fuzzing {
}

but given (C):

#include <stdint.h>
namespace fuzzing {


}

would be formatted to (D):

#include <stdint.h>
namespace fuzzing {

}

The invocation specifically is clang-format --style=google file. Prior to this patch, both inputs (A/C) would give the same output:

#include <stdint.h>
namespace fuzzing {}

Is this unintended behavior and worth looking into? We have tests for generated code that use clang-format to attempt to "unify" expected and generated output for easy comparison. We would expect that extra whitespace not produce different formatted outputs. Thanks.

That's definitely an unintended behaviour. Please file a bug and possibly mark it as release blocker for LLVM 12. Either a fix will be there soon, or we'll revert.

FYI, simple reproduce:

verifyFormat("#include <stdint.h>\n"
             "namespace rep {}",
             Style);

The problem doesn't happen with #include "stdint.h".

clang/lib/Format/UnwrappedLineFormatter.cpp
374–375

@leonardchan, it seems this is the culprit. the closing > in the #include <...> is treated as if it were a template closer and so it inhibits line merging.

That's definitely an unintended behaviour. Please file a bug and possibly mark it as release blocker for LLVM 12. Either a fix will be there soon, or we'll revert.

FYI, simple reproduce:

verifyFormat("#include <stdint.h>\n"
             "namespace rep {}",
             Style);

The problem doesn't happen with #include "stdint.h".

Thanks for looking into this. I filed https://bugs.llvm.org/show_bug.cgi?id=48891.