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

Manuel Klimek via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Mar 18 01:43:41 PDT 2019


klimek added a comment.

In D55170#1431854 <https://reviews.llvm.org/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 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.
>
> So is this a personal opinion or based on a technical reason (other than the normal mantra about risk & cost),


I don't think risk is what matters - in the end it's about cost, and cost is a very technical reason.

>   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

I'm fine having a discussion - my main question is why this matters. This seems a lot more arbitrary than other things we have.

> @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.




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55170/new/

https://reviews.llvm.org/D55170





More information about the cfe-commits mailing list