Page MenuHomePhabricator

clang-format: add option to merge empty function body
ClosedPublic

Authored by Typz on May 23 2017, 9:39 AM.

Details

Summary

This option supplements the AllowShortFunctionsOnASingleLine flag, to
merge empty function body at the beginning of the line: e.g. when the
function is not short-enough and breaking braces after function.

int f()
{}

Diff Detail

Repository
rL LLVM

Event Timeline

Typz created this revision.May 23 2017, 9:39 AM
Typz added inline comments.May 23 2017, 9:41 AM
unittests/Format/FormatTest.cpp
6029 ↗(On Diff #99934)

When testing this patch, I found a bug when AllowShortFunctionsOnASingleLine = SFS_Empty and MergeEmptyOnly.BraceWrapping.AfterFunction = true : I did not have the time to fix it yet, any idea what might be causing this?

Typz added inline comments.May 23 2017, 12:04 PM
include/clang/Format/Format.h
158 ↗(On Diff #99934)

maybe this should be a nested option inside BraceWrapping?

or this should be donc implicit when breaking after function (BraceWrapping.AfterFunction = true) and AllowShortFunctionOnASingleLine (hence no option added) ? Or even AllowShortBlocksOnASingleLine (though the doc limits this option to statements...)

unittests/Format/FormatTest.cpp
6131 ↗(On Diff #99934)

missing test for parsing YAML option

Typz marked 2 inline comments as done.May 24 2017, 2:02 AM
Typz updated this revision to Diff 100053.May 24 2017, 2:03 AM

Fix SFS_Empty & SFS_Inline merging of empty function when BraceWrapping.AfterFunction, and add missing test

djasper edited edge metadata.May 24 2017, 2:56 AM

Does anything speak against making this behavior happen with AllowShortFunctionsOnASingleLine = SFS_Empty and MergeEmptyOnly.BraceWrapping.AfterFunction = true? I mean without the extra style option?

unittests/Format/FormatTest.cpp
6067 ↗(On Diff #100053)

indent

Typz added a comment.May 24 2017, 3:10 AM

Does anything speak against making this behavior happen with AllowShortFunctionsOnASingleLine = SFS_Empty and MergeEmptyOnly.BraceWrapping.AfterFunction = true? I mean without the extra style option?

That is fine with me (with AllowShortFunctionsOnASingleLine >= SFS_Empty condition), but I see two things:

  • it does not match the "OnASingleLine" part of that option's name, since in that case we get the function definition on one line, and the body on the next line
  • some (existing) coding style may use BraceWrapping.AfterFunction and AllowShortFunctionsOnASingleLine (for exemple Mozilla), but may not want to change the behavior when the function cannot be merged on a single line
Typz marked an inline comment as done.May 24 2017, 3:11 AM
Typz updated this revision to Diff 100060.May 24 2017, 3:11 AM

fix indent

As it currently stands, I am really not happy with the configuration space that this opens up. If we can't make the configuration of existing flags, what's the coding style encourages this behavior?

Typz added a comment.May 24 2017, 4:33 AM

Facebook's HHVM seems to use this for empty constructors: https://github.com/facebook/hhvm/blob/master/hphp/doc/coding-conventions.md
This is also done systematically in Qt and QtCreator code.

Personally, I don't care if this is an option or not; but I fear it would break existing styles.

But that style specifically says that it is only done if the initializer list is wrapped:
https://github.com/facebook/hhvm/blob/master/hphp/doc/coding-conventions.md#constructor-initializer-lists

I.e. we would do the right thing for that style if we would set BraceWrapping.AfterFunction to true and AllowShortFunctionsOnASingleLine to SFS_Empty and it would then format:

Constructor() {}
Constructor() //
    : initializer(..)
{}
Typz added a comment.May 24 2017, 5:15 AM

I am fine with "removing" the options: I only fear it would incorrectly affect existing users/styles, which would possibly end up in the patch being reverted...
I could not find the info in Mozilla coding style, but looking at the code it seems it should be done indeed, but with an extra space in between, like this:

void foo()
{ }

So if you tell me it's fine I will go ahead and replace the option with the BraceWrapping.AfterFunction && AllowShortFunctionsOnASingleLine >= SFS_Empty condition.

No, I don't think it should be done this way and neither Facebook nor Mozilla coding styles say you should.

Mozilla style has an explicit example:

int TinyFunction() { return mVar; }

Facebook style has an explicit example:

MyClass::MyClass(uint64_t idx) : m_idx(idx) {}

Moving the "{}" to the next line is only ok if the complete function/constructor definition (up to and including the "}") does not fit on one line.

Typz added a comment.May 24 2017, 5:55 AM

Right, I was not clear enough: this is indeed done only if everything does not fit on one line.

There is an exemple in facebook's coding style:

MyClass::MyClass(const Class* cls, const Func* func, const Class* ctx)
  : m_cls(cls)
  , m_func(func)
  , m_ctx(ctx)
  , m_isMyConditionMet(false)
{}

And this happens (with the extra space) in the same kind of situation in Mozilla's code:

nsSameProcessAsyncMessageBase::nsSameProcessAsyncMessageBase(JS::RootingContext* aRootingCx,
                                                             JS::Handle<JSObject*> aCpows)
  : mRootingCx(aRootingCx)
  , mCpows(aRootingCx, aCpows)
{ }
Typz added a comment.May 24 2017, 6:17 AM

Merging empty blocks is not allowed in webkit style: https://webkit.org/code-style-guidelines/#punctuation-member-init

So it seems changing the behavior would actually fix mozilla style and break webkit style (as integrated in clang-format).

I don't understand. WebKit style would not set AllowShortFunctionsOnASingleLine and so the behavior there wouldn't change, I presume?

Typz added a comment.May 26 2017, 5:56 AM

Webkit inherits AllowShortFunctionsOnASingleLine = FormatStyle::SFS_All from LLVM style, so merging the options would introduce these explicitely-forbidden empty blocks.
But the empty blocks should actually be used in code following Mozilla or Qt style.

Typz updated this revision to Diff 100393.May 26 2017, 5:58 AM

update mozilla style to use empty function blocks

I think it's just wrong that WebKit inherits this. The style guide explicitly says that this is wrong:

MyOtherClass::MyOtherClass() : MySuperClass() {}

So I still think we can make this work with the existing options without regressing a behavior that anyone is relying on.

Typz added a comment.EditedMay 26 2017, 7:01 AM

I think it's just wrong that WebKit inherits this. The style guide explicitly says that this is wrong:

MyOtherClass::MyOtherClass() : MySuperClass() {}

I think this exemple applies to constructors only (and it works even with inlining, since this style has BreakConstructorInitializersBeforeComma = true and ConstructorInitializerAllOnOneLineOrOnePerLine = false, so there is already a line break before the colon)

So I still think we can make this work with the existing options without regressing a behavior that anyone is relying on.

Looking at webkit code, there are many short functions which are indeed formatted on a single line (at least 22097 occurences, actually), so removing AllowShortFunctionsOnASingleLine from webkit style does not seem appropriate.

Lets try this the other way around. I am not ok with introducing an additional top-level option for this. It simply isn't important enough. So find a way for the existing style flags to support what you need and not regress existing users. If that can't be done, I am also ok with adding another value into BraceWrapping (which you suggested at some point, I think).

Typz updated this revision to Diff 100412.May 26 2017, 8:13 AM

move option to BraceWrapping

djasper added inline comments.May 26 2017, 8:42 AM
include/clang/Format/Format.h
644 ↗(On Diff #100412)

Reflow the comment.

654 ↗(On Diff #100412)

I think the name probably isn't very intuitive. Maybe invert it and call it "SplitEmptyFunctionBody"?

unittests/Format/FormatTest.cpp
6092 ↗(On Diff #100412)

indent. Here and elsewhere.

Typz updated this revision to Diff 100420.May 26 2017, 8:54 AM
Typz marked 2 inline comments as done.

fix indent & rename option to SplitEmptyFunctionBody

Typz marked an inline comment as done.May 26 2017, 8:55 AM
djasper accepted this revision.Jun 12 2017, 7:43 AM

Looks good.

This revision is now accepted and ready to land.Jun 12 2017, 7:43 AM
This revision was automatically updated to reflect the committed changes.