[PATCH] D110833: [clang-format] Refactor SpaceBeforeParens to add options

[Christian Rayroud via Phabricator via cfe-commits]

crayroud marked 6 inline comments as done.
crayroud added inline comments.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3708
+    SpaceBeforeParens: Custom
+    SpaceBeforeParensFlags:
+      AfterFunctionDefinitionName: true
----------------
MyDeveloperDay wrote:
> I'm not a massive fan of the use of 'Flags' in the config (I know we use it as the typename), naming things is hard!
SpaceBeforeParensFlags has been renamed to SpaceBeforeParensOptions.


================
Comment at: clang/include/clang/Format/Format.h:3416
+  /// \version 14
+  SpaceBeforeParensCustom SpaceBeforeParensFlags;
+
----------------
MyDeveloperDay wrote:
> HazardyKnusperkeks wrote:
> > MyDeveloperDay wrote:
> > > I'm not a massive fan of the word `Flags` here and thoughts?
> > Yeah, neither, but Options is taken.
> > 
> > But Flags indicate that these will always be booleans, and we know that may change in the future.
> > 
> > Is it possible the reuse SpaceBeforeParensOptions as struct and still parse the old enum? (Yes of course, but is it feasible in terms of needed work?)
> but "Options" as in SpaceBeforeParensOptions  is the type not the name so we could use SpaceBeforeParensOptions 
> 
> personally I would change the typename of SpaceBeforeParensOptions  to avoid confusion but its not essential as it would be 
> 
> `SpaceBeforeParensCustom  SpaceBeforeParensOptions;`
> 
> for me I sometimes like using Struct anyway
> 
> `SpaceBeforeParensStruct SpaceBeforeParensOptions;`
SpaceBeforeParensFlags has been renamed to SpaceBeforeParensOptions. And the old SpaceBeforeParensOptions enum has been renamed to SpaceBeforeParensStyle.


================
Comment at: clang/unittests/Format/FormatTest.cpp:14193
+  verifyFormat("M (std::size_t R, std::size_t C) : C(C), data(R) {}",
+               SomeSpace2);
 }
----------------
MyDeveloperDay wrote:
> IMHO I think we should see tests for the other combinations of custom (I know it might be repeated)
More tests has been added


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

https://reviews.llvm.org/D110833