This revision was automatically updated to reflect the committed changes.
Closed by commit rL305272: clang-format: add option to merge empty function
body (authored by Typz).
Changed prior to commit:
https://reviews.llvm.org/D33447?vs=100420&id=102294#toc
Repository:
rL LLVM
https://reviews
djasper accepted this revision.
djasper added a comment.
This revision is now accepted and ready to land.
Looks good.
https://reviews.llvm.org/D33447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listi
Typz added a comment.
ping?
https://reviews.llvm.org/D33447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Typz added a comment.
ping
https://reviews.llvm.org/D33447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Typz updated this revision to Diff 100420.
Typz marked 2 inline comments as done.
Typz added a comment.
fix indent & rename option to SplitEmptyFunctionBody
https://reviews.llvm.org/D33447
Files:
include/clang/Format/Format.h
lib/Format/Format.cpp
lib/Format/UnwrappedLineFormatter.cpp
u
djasper added inline comments.
Comment at: include/clang/Format/Format.h:644
+/// This option is used only if the opening brace of the function has
+/// already
+/// been wrapped, i.e. the `AfterFunction` brace wrapping mode is set, and
Reflow the com
Typz updated this revision to Diff 100412.
Typz added a comment.
move option to BraceWrapping
https://reviews.llvm.org/D33447
Files:
include/clang/Format/Format.h
lib/Format/Format.cpp
lib/Format/UnwrappedLineFormatter.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatT
djasper added a comment.
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 al
Typz added a comment.
In https://reviews.llvm.org/D33447#765610, @djasper wrote:
> 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.
djasper added a comment.
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 relyi
Typz updated this revision to Diff 100393.
Typz added a comment.
update mozilla style to use empty function blocks
https://reviews.llvm.org/D33447
Files:
include/clang/Format/Format.h
lib/Format/Format.cpp
lib/Format/UnwrappedLineFormatter.cpp
unittests/Format/FormatTest.cpp
Index: uni
Typz added a comment.
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.
https://reviews.l
djasper added a comment.
I don't understand. WebKit style would not set AllowShortFunctionsOnASingleLine
and so the behavior there wouldn't change, I presume?
https://reviews.llvm.org/D33447
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
h
Typz added a comment.
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).
https://reviews.llvm.org/D334
Typz added a comment.
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)
djasper added a comment.
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(id
Typz added a comment.
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, bu
djasper added a comment.
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
BraceWrapp
Typz added a comment.
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
djasper added a comment.
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?
https://reviews.llvm.org/D33447
___
Typz updated this revision to Diff 100060.
Typz added a comment.
fix indent
https://reviews.llvm.org/D33447
Files:
include/clang/Format/Format.h
lib/Format/Format.cpp
lib/Format/UnwrappedLineFormatter.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
==
Typz added a comment.
In https://reviews.llvm.org/D33447#763045, @djasper wrote:
> Does anything speak against making this behavior happen with
> AllowShortFunctionsOnASingleLine = SFS_Empty and
> MergeEmptyOnly.BraceWrapping.AfterFunction = true? I mean without the extra
> style option?
Tha
djasper added a comment.
Does anything speak against making this behavior happen with
AllowShortFunctionsOnASingleLine = SFS_Empty and
MergeEmptyOnly.BraceWrapping.AfterFunction = true? I mean without the extra
style option?
Comment at: unittests/Format/FormatTest.cpp:6067
+
Typz updated this revision to Diff 100053.
Typz added a comment.
Fix SFS_Empty & SFS_Inline merging of empty function when
BraceWrapping.AfterFunction, and add missing test
https://reviews.llvm.org/D33447
Files:
include/clang/Format/Format.h
lib/Format/Format.cpp
lib/Format/UnwrappedLine
Typz added inline comments.
Comment at: include/clang/Format/Format.h:158
+ ///
+ bool AllowEmptyFunctionBodyOnASingleLine;
+
maybe this should be a nested option inside BraceWrapping?
or this should be donc implicit when breaking after function
(BraceWrappin
Typz added inline comments.
Comment at: unittests/Format/FormatTest.cpp:6029
+
+ //TODO: this case does not work, not sure if there is reason...
+ MergeEmptyOnly.BreakBeforeBraces = FormatStyle::BS_Custom;
When testing this patch, I found a bug when `AllowShort
Typz created this revision.
Herald added a subscriber: klimek.
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()
{}
https://revi
27 matches
Mail list logo
