Page MenuHomePhabricator

Fixes bug 20587 - Add K&R break before braces style
ClosedPublic

Authored by lifted on Aug 9 2014, 9:40 AM.

Details

Summary

http://llvm.org/bugs/show_bug.cgi?id=20587

Added K&R style. It could be enabled by the following option:

BreakBeforeBraces: KernighanRitchie

This style is like Attach, but break *only* before function
declarations.

As I can see, no additional logic required to support this style, any
style different from other styles automagically satisfies K&R.

Diff Detail

Event Timeline

lifted updated this revision to Diff 12325.Aug 9 2014, 9:40 AM
lifted retitled this revision from to Fixes bug 20587 - Add K&R break before braces style.
lifted updated this object.
lifted added a reviewer: djasper.
lifted added subscribers: klimek, Unknown Object (MLST).
djasper accepted this revision.Aug 9 2014, 1:47 PM
djasper edited edge metadata.

LOL.. Very interesting that no additional logic is necessary.

Looks good with a few minor comments.

docs/ClangFormatStyleOptions.rst
176 ↗(On Diff #12325)

Please generate this with docs/tools/dump_format_style.py. Otherwise, the next re-generation of this file will contain unrelated changes.

include/clang/Format/Format.h
287 ↗(On Diff #12325)

"named"?

Also, maybe "..., but only break ..."?

unittests/Format/FormatTest.cpp
8088

Call this KRStyle?

This revision is now accepted and ready to land.Aug 9 2014, 1:47 PM
lifted updated this revision to Diff 12332.Aug 10 2014, 12:10 AM
lifted edited edge metadata.
  • Regenerate ClangFormatStyleOptions.rst with a tool
  • Fix description of the BS_KernighanRitchie
  • Add a test case to check formatting of lambdas
lifted updated this revision to Diff 12333.Aug 10 2014, 12:39 AM

Minor changes in Format.h to synchronize it with ClangFormatStyleOptions.rst.

I have done a bit more reading and now I am thoroughly confused:

  1. I think Linux uses K&R style. So we should not need to introduce a new style but fix Linux style instead.
  2. According to all resources I have found, K&R style also puts the "{" on the new line for classes and namespaces.
  3. Neither Linux nor K&R style should but the "{" on the new line for structs, so we will need to handle structs and classes differently.

Does that make sense?

djasper requested changes to this revision.Aug 10 2014, 2:02 AM
djasper edited edge metadata.
This revision now requires changes to proceed.Aug 10 2014, 2:02 AM

I think Linux uses K&R style. So we should not need to introduce a new style but fix Linux style instead.

Yep, that's what I wrote in the original bug. I don't mind fixing linux style. But it could break backward compatibility.

According to all resources I have found, K&R style also puts the "{" on the new line for classes and namespaces.

The only resource I've found is AStyle Manual.
Original K&R book contains no classes nor namespaces, so everyone is free to interpret K&R style for C++ constructs as they like. I wish I could ask Ritchie how would he place braces...
My opinion is that Stroustrup style is the closest one to original K&R style, it just adds a few more breaks for control structures.

we will need to handle structs and classes differently.

I don't think it's a good idea. From C++ point of view, struct is almost identical to class. If formatter changes the breaks when I replace struct with a class, it will confuse me much. I don't know anyone who wish to format structs and classes differently.

Possibly, it's better to provide fine-grained control over the brace usage (break after struct/class/) like Uncrucify do. The BreakBeforeBrace could then be high-level interface for fine-grained options.

include/clang/Format/Format.h
287 ↗(On Diff #12325)

I used "named" here because I'm not sure how to format lambdas in K&R. I think it's perfectly legal to attach braces to lambda declarations because lambdas could be nested (unlike named functions).
Either way, comments for other styles don't stress this, so I'll remove "named".

Ok. I have also come to the conclusion that it is best to have one higher level style choice (I'd name this Linux style and fix it similar to what the K&R style from this patch does) and then give options to "override" each individual brace choice.

Can you fix the Linux style in this patch, basically making it what K&R style does now? We can then add the further options later or when requested. I actually do know codebases where structs are formatted with the brace attached whereas classes are not :-(.

lifted added a comment.EditedAug 11 2014, 12:19 AM

Can you fix the Linux style in this patch, basically making it what K&R style does now?

Ok.

actually do know codebases where structs are formatted with the brace attached whereas classes are not :-(.

Possibly, they use AStyle for formatting :) I think fine-grained options is a good solution for such codebases. In addition, it's still trivial enough to provide "ArtisticStyleLinux" option to support the codebases.

djasper added inline comments.Aug 11 2014, 1:01 AM
unittests/Format/FormatTest.cpp
8091

Can you add small test for structs just so that we realize when we are changing the behavior there?

I have done the same for Kernel style and it seems the pretty consistency attach the "{" to structs.. So maybe we should just leave structs alone for now :-(.

Well, just to clarify:
Are we decided to follow AStyle way and break after classes and namespaces, but not before structs?

unittests/Format/FormatTest.cpp
8091

Ok, I'll add one.

I think so. Not sure whether it makes sense, but there are definitely arguments for it. astyle conformance is one, but more importantly structs are more of a C-concept and K&R style (well the book) is pretty consistent about attaching the "{" to the struct. Treating the C++ concepts of namespaces and classes differently seems inconsistent but what many projects do.

lifted updated this revision to Diff 12343.Aug 11 2014, 4:30 AM
lifted edited edge metadata.

Fix Linux BrakeBeforeBraces style: attach left braces to structs as
shown to us by the prophets Kernighan and Ritchie.

Before:

namespace ns
{
class C
{
public:
  int x;
};
struct X
{
  int x;
};
}

After:

namespace ns
{
class C
{
public:
  int x;
};
struct X {
  int x;
};
}
djasper accepted this revision.Aug 11 2014, 4:48 AM
djasper edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Aug 11 2014, 4:48 AM
lifted closed this revision.Aug 11 2014, 5:27 AM

Ouch, the arc commit used the original review summary and didn't allow me to edit the commit message :(
I'm really sorry about it.