Differential D55170
[clang-format]: Add NonEmptyParentheses spacing option Authored by reuk on Dec 1 2018, 3:25 PM.
Details This patch aims to add support for the following rules from the JUCE coding standards:
The entire set of JUCE coding guidelines can be found here. Unfortunately, clang-format can't enforce all of these style rules at the moment, so I'm trying to add support for them. Patch by Reuben Thomas
Diff Detail
Event TimelineComment Actions 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 might help if your changes didn't also include additional unrelated clang-formatting changes because its a little harder to see what actually changed, and your review would be much smaller. perhaps the next steps could be:
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::kw_while,
tok::kw_switch, tok::kw_case, TT_ForEachMacro,
TT_ObjCForIn) ||
Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
(Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
tok::kw_new, tok::kw_delete) &&
(!Left.Previous || Left.Previous->isNot(tok::period))))) ||
(Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
(Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
Left.is(tok::r_paren) ||
(Left.Tok.getIdentifierInfo() &&
Left.Tok.getIdentifierInfo()->isKeyword(
getFormattingLangOpts(Style)))) &&
Line.Type != LT_PreprocessorDirective) ||
(Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
Right.ParameterCount >= 1 &&
(Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
Left.is(tok::r_square) || Left.is(tok::r_paren) ||
(Left.Tok.getIdentifierInfo() &&
Left.Tok.getIdentifierInfo()->isKeyword(
getFormattingLangOpts(Style)))) &&
Line.Type != LT_PreprocessorDirective);There is nothing wrong with it per say I'm sure it works beautifully but its a little hard to reason about. I know you only adding to the end of it, but I think if this was in clang-tidy we'd be asked to write it as a function/matcher to encapsulate this kind of functionality. Its also an indictment of clang format that this isn't aligning in such a way that it can be more easily read, I mean I know its done its job, but its quite a challenge to look at! ;-) To me clang-format needs more functions like Left.isIdentifier() Left.isParen() Left.isLSquare() Right.hasParameters() in order to help break complex conditional like this down into a readable form which can be rationalised about. This would help hide some of the tok::XXX complexity which can make my eyes at least go funny.. and code like this (which I think you added) Left.Tok.getIdentifierInfo() && Left.Tok.getIdentifierInfo()->isKeyword(getFormattingLangOpts(Style))) (especially as its repeated in the clause) It looks like it means some means something functional which perhaps could be combined: (sorry I don't know what its doing but I think you do) isLanguageKeyWord(Left,Style) or Left.isLanguageKeyWord(Style); If you could add the part you added, is a more succinct way, || isFunctionWithNoArguments(Left,Right) rather than ||
(Left.Tok.getIdentifierInfo() &&
Left.Tok.getIdentifierInfo()->isKeyword(
getFormattingLangOpts(Style)))) &&
Line.Type != LT_PreprocessorDirective) ||
(Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
Right.ParameterCount >= 1 &&
(Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
Left.is(tok::r_square) || Left.is(tok::r_paren) ||
(Left.Tok.getIdentifierInfo() &&
Left.Tok.getIdentifierInfo()->isKeyword(
getFormattingLangOpts(Style)))) &&
Line.Type != LT_PreprocessorDirective);I think there would be a stronger argument that it should go in, given that you have a publicly available style guide etc... (owners may not agree!) Perhaps too much of my own opinion here... but maybe the "development costs" would go down if thing like Left.endsSequence(tok::kw_constexpr, tok::kw_if) where written as Left.isConstExprIf() if that is what its doing? Comment Actions sorry to be critical, just trying to help get traction on your review...
Comment Actions 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 not sure whether a more general tidy-up falls within the scope of this PR. Comment Actions 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 even bug fixes reviewed! Thank you for breaking out the common code, I do think more functions like this in clang-format would help make the code more readable. I also think the various if clauses having comment helps break up these huge monolithic if statements Thanks for the patch. Comment Actions 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? Comment Actions I'd actively encourage you to go get commit access, I think its much better when the commit comes from the author. There just isn't enough people actively involved in clang-format (and other tools) for us to get reviews even looked at (even for defects), we need more people involved fixing defects and doing reviews, getting commit access shows an intent to fix anything in that comes from your own review which is one of the things the code owners push as being an objection to adding more code. Comment Actions 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 come up with random variance. I do not think this option pulls its weight. Comment Actions Pulling this revision down to build it caused build issues with missing SBBLO_Always, could you investigate before committing?
Comment Actions @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 the most recent clang master and updated llvm. I've been testing/building with this command: cmake --build build --target FormatTests && ./build/tools/clang/unittests/Format/FormatTests && cmake --build build --target clang-format @klimek That's disappointing, I thought other JUCE users might benefit from this patch (it's quite an established library), and the amount of code that I'm a adding is not that high. Comment Actions 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/Format.h$1578 Comment Actions
So is this a personal opinion or based on a technical reason (other than the normal mantra about risk & cost), Could we give @reuk some positive feedback about what he needs to change in this review to get this accepted? He has the other boxes checked about public style etc.. It may not be your preference but its not without some merit for those of us who like to fine tune our style, if that can be done with minimal impact to other styles and is well covered by tests, then can we at least have some discussion first @reuk please lets continue to fix this revision so it builds as I want to pull it into https://github.com/mydeveloperday/clang-experimental As an aside, Wouldn't it be great if JUCE was a public style so you could just say BasedOnStyle: JUCE Where that style defines what JUCE uses? I've always found this odd, I've never understood why we only have Google,WebKit,Mozilla,LLVM, I would have thought it would have been nice to have other big guns Microsoft,Qt generally its only going to be a single function setting the defaults. I actually think a change like that would really help people so they don't have to work out all the individual styles, it would help keep .clang-format files small and neat and not full of a bunch of different options. Even if JUCE was based on another style say "Google" the code should really be saying if (Style.isGoogleDerivedStyle()) ...... and not Style == FormatStyle::Google When its specific to JUCE it would say if (Style.isJUCEDerivedStyle()) ...... I know when using LLVM, that being able to have a .clang-format file that just says BasedOnStyle: LLVM is really nice, I don't have to go work out what the LLVM style is, I just get what the project defines, I think it would be great that anything that passes the public style test for submissions should really be able to add that kind of configuration. Comment Actions @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. Comment Actions I don't think risk is what matters - in the end it's about cost, and cost is a very technical reason.
I'm fine having a discussion - my main question is why this matters. This seems a lot more arbitrary than other things we have.
Comment Actions @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 contributing to JUCE and I thought others might too. Comment Actions
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 mean a financial cost? could you define cost as a problem so we can address it? what would it take to make cost not an issue? Comment Actions 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. Comment Actions 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 more important. To make cost not an issue we'd need to change clang-format to make changes like this significantly simpler to review & maintain. I'll add a code comment in a moment, given that this is probably something that we'll allow given it fulfills the criteria we've set ourselves.
Comment Actions IDK. If the criteria is "public coding standard for widely used project", supporting all such things may be unsustainable. Some projects may need to fork or find another formatter. I speak as someone that maintains an internal fork with features that I do not deem worthy of foisting on trunk.
Comment Actions
Don't get me wrong I totally appreciate what you do, But what you mean is it costs your employer. That I understand, but not all of us are doing this on behalf of a company (more specially not yours), so you could also say that your employer benefits the other way from those contributors who give their time without them having to spend a dime. I guess my question would be, should the cost to your employer really come into the decision about whether something goes in or not? Other than of course we are totally grateful to them for giving us so much of your time, but that shouldn't have impact on what is worthy to go in should it? or am I wrong?
Comment Actions This is not a good argument. There are likely always ways to implement the same external side-effects
Comment Actions @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 before, namely Style.SpaceBeforeParens == FormatStyle::SBPO_Always Now that clause is abstracted away to be (the abstraction in my view being a nice part of this change, becuase it avoided duplication and made it easier to reason about!) Style.SpaceBeforeParens == FormatStyle::SBPO_Always || (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses && Right.ParameterCount > 0); i.e. always add a space, or add a space only when the number of parameters is > 0 and I don't want a space on empty parameters. (simple enough!) So whilst I understand the arguments of code complexity, I feel it is sometimes used to discourage, what is in at least my view seemingly reasonable requests. (not my choice but reasonable all the same) This was why I asked what was meant by "cost", but you raise the other cost, and that is one that is more legitimate... or is it?. have we seen it here as a problem or do we just fear it? Having come from almost nothing to understanding a certain amount of of the code recently in a relatively short space of time (enough to at least contribute a few bug fixes), I'm confident that the "up to speed" costs are actually currently relatively low, and as the Format Library is only 20,000 lines and Format.cpp being only ~2500, I don't consider that to be large enough for the code to have come intractable already. Of course I understand when someone starts asking for an Option e.g. AllowSpaceAfterSecondCommaOfArgumentListButNotBeforeAnyOfTheOthers: true We may have gone too far ;-), but I almost think that perhaps those cases might be more limited than we think in the end. From my recent experiences one nod to complexity was from the C++ language itself.. when trying to fix a bug in isFunctionDeclarationName() e,g, A foo(abc); does the mean a function foo() taking an argument abc and returning A or an instance of class A called foo being constructed with an argument abc.. This can have all the difference if you want to put a BraceAfterReturnType The "cost" as you describe it here, over our ability to reason about the code in my view is more likely to come from having to analyze snippets of languages without the full parser to reason with. Comment Actions This adds another condition to an already complex "if" that I think literally nobody fully understands, and I think it does so for very questionable value (why does it matter whether there is a space diff between the arg and no-arg case?).
We *have* to offset this against the value of a feature, especially in a bikeshed coloring tool like clang-format.
I am working on a patch (for over a year now :( to generalize macro configuration instead of having special cases that don't really work well, and it's much harder than it should be, as the architecture is getting in my way.
Sure, that's where large part of the system complexity comes from, as tackling this is not easy.
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||