[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

This revision was automatically updated to reflect the committed changes. Closed by commit rL357344: [clang-format]: Add NonEmptyParentheses spacing option (authored by reuk, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

klimek accepted this revision. klimek added a comment. Oh, and LG :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55170/new/ https://reviews.llvm.org/D55170 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

klimek added a comment. In D55170#1439864 , @MyDeveloperDay wrote: > @lebedev.ri Are we talking about a general ideology of the long term cost to > allow any new things in? or to not allow things in this specific case? > > because in this specific case a

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

MyDeveloperDay added a comment. @lebedev.ri Are we talking about a general ideology of the long term cost to allow any new things in? or to not allow things in this specific case? because in this specific case all the changes are based on what is really a single clause that was already there be

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

lebedev.ri added a comment. In D55170#1436732 , @MyDeveloperDay wrote: > > The cost is financial, as it's developer time, which costs real money to > > companies. In the end, to support this, people like myself who are doing > > this as part of their jo

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

reuk updated this revision to Diff 191560. reuk added a comment. Removed unnecessary parens. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55170/new/ https://reviews.llvm.org/D55170 Files: include/clang/Format/Format.h lib/Format/Format.cpp lib/Format/TokenAnnotator.cpp lib/For

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

MyDeveloperDay added a comment. > The cost is financial, as it's developer time, which costs real money to > companies. In the end, to support this, people like myself who are doing this > as part of their job spend time that they'd otherwise spend to make other > things better that might be mo

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

klimek added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:2546-2560 return Line.Type == LT_ObjCDecl || Left.is(tok::semi) || (Style.SpaceBeforeParens != FormatStyle::SBPO_Never && -(Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

reuk marked an inline comment as done. reuk added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:2546-2560 return Line.Type == LT_ObjCDecl || Left.is(tok::semi) || (Style.SpaceBeforeParens != FormatStyle::SBPO_Never && -(Left.isOneOf(tok::

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

mzeren-vmw added a comment. In D55170#1436087 , @klimek wrote: > Oh wow, wasn't aware how popular this is. Sigh. I'd like us to push them to > change their style guide, but I guess that's not going to fly. IDK. If the criteria is "public coding standard

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

klimek added a comment. In D55170#1432929 , @MyDeveloperDay wrote: > > I don't think risk is what matters - in the end it's about cost, and cost > > is a very technical reason > > I'm really sorry I'm not trying to be difficult its just over the years I

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

klimek added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:2546-2560 return Line.Type == LT_ObjCDecl || Left.is(tok::semi) || (Style.SpaceBeforeParens != FormatStyle::SBPO_Never && -(Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

klimek added a comment. In D55170#1432832 , @reuk wrote: > @klimek I agree that the rule is somewhat arbitrary. However, it's the style > rule of an established codebase with many users (I don't have a concrete > number, but the project has 1400 stars on

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

MyDeveloperDay accepted this revision. MyDeveloperDay added a comment. This revision is now accepted and ready to land. LGTM (landed here too https://github.com/mydeveloperday/clang-experimental/releases) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55170/new/ https://reviews.llvm.org

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

MyDeveloperDay added a comment. > I don't think risk is what matters - in the end it's about cost, and cost is > a very technical reason I'm really sorry I'm not trying to be difficult its just over the years I keep seeing this being said in reviews? but what is the cost? I assume you don't

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

reuk added a comment. @klimek I agree that the rule is somewhat arbitrary. However, it's the style rule of an established codebase with many users (I don't have a concrete number, but the project has 1400 stars on github ). I've found this patch useful when c

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

klimek added a comment. In D55170#1431854 , @MyDeveloperDay wrote: > > I'm sorry for not having a positive answer here, but I'm not in favor of > > this change. The style rule looks like it introduces arbitrary complexity > > at a point where I don't un

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

reuk updated this revision to Diff 191050. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55170/new/ https://reviews.llvm.org/D55170 Files: include/clang/Format/Format.h lib/Format/Format.cpp lib/Format/TokenAnnotator.cpp lib/Format/TokenAnnotator.h unittests/Format/FormatTest.c

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

MyDeveloperDay added inline comments. Comment at: include/clang/Format/Format.h:1578 /// \endcode bool SpaceBeforeCpp11BracedList; boolean here not enum see comment below Comment at: lib/Format/TokenAnnotator.cpp:2608 +(Style.Sp

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

reuk updated this revision to Diff 191015. reuk added a comment. @MyDeveloperDay I'm sorry, you're absolutely correct. I'd got some other JUCE-related changes mixed up in this PR. Should be fixed now. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55170/new/ https://reviews.llvm.org/D55

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

MyDeveloperDay added a comment. > I'm sorry for not having a positive answer here, but I'm not in favor of this > change. The style rule looks like it introduces arbitrary complexity at a > point where I don't understand at all how it matters. We cannot possibly > support all style guides that

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

MyDeveloperDay added a comment. I'm happy to be wrong, but In current master, SpaceBeforeCpp11BracedList is a boolean not an enum (but I think I've seen a review changing it somewhere which maybe isn't landed) https://reviews.llvm.org/source/llvm-github/browse/master/clang/include/clang/Format

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

reuk added a comment. @MyDeveloperDay I'm not sure you built this branch. Perhaps you applied this patch to an older version of the repo. For me, SpaceBeforeCpp11BracedListOptions is defined at Format.h:1570. This builds fine for me, on macOS 10.14 with Xcode 10.1's clang. I just rebased onto

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

MyDeveloperDay requested changes to this revision. MyDeveloperDay added a comment. This revision now requires changes to proceed. Pulling this revision down to build it caused build issues with missing SBBLO_Always, could you investigate before committing? Comment at: lib/Form

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

klimek added a comment. In D55170#1423941 , @MyDeveloperDay wrote: > In D55170#1423798 , @reuk wrote: > > > Thanks for the review, and for approving this PR. It's very much > > appreciated! > > > > I don't have me

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

MyDeveloperDay added a comment. In D55170#1423798 , @reuk wrote: > Thanks for the review, and for approving this PR. It's very much appreciated! > > I don't have merge rights - could someone commit this for me please? I'd actively encourage you to go get

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

reuk added a comment. Thanks for the review, and for approving this PR. It's very much appreciated! I don't have merge rights - could someone commit this for me please? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55170/new/ https://reviews.llvm.org/D55170 _

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

MyDeveloperDay accepted this revision. MyDeveloperDay added a comment. This revision is now accepted and ready to land. I'm not the code owner but this LGTM, Assuming the unit tests remain passing, I'd like to see more items like this in clang-format addressed, at present we seem to lack getting

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

reuk updated this revision to Diff 189942. reuk added a comment. I've rebased onto master, and removed unrelated formatting changes. I've also tried to remove some of the duplicate parens-related expressions. I agree that the heavy nested boolean expressions are a bit painful to read, but I'm n

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

MyDeveloperDay added a comment. sorry to be critical, just trying to help get traction on your review... Comment at: lib/Format/TokenAnnotator.cpp:65 -const FormatToken &Previous = *CurrentToken->Previous; // The '<'. +const FormatToken &Previous = *CurrentToken->Pre

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

MyDeveloperDay added a comment. Hi @reuk, I'm trying to go over old reviews and see if they are still desired, I stumbled on your review and want to see if its still important. Here is some initial feedback. I know its ironic that the clang-format code isn't clang-formatted itself, but it mig

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

reuk added a subscriber: klimek. reuk added a comment. Would someone review this please? I'm not sure who to add for review (sorry), maybe one of the following? @klimek @Typz @krasimir Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55170/new/ https://reviews.llvm

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

reuk created this revision. reuk added reviewers: Typz, krasimir, cfe-commits. This patch aims to add support for the following rules from the JUCE coding standards: - Always put a space before an open parenthesis that contains text - e.g. foo (123); - Never put a space before an empty pair of