[clang-format] Add an option to add a space between operator overloading and opening parentheses

[rajatbajpai]

Summary

This change adds an option AfterOverloadedOperator in SpaceBeforeParensOptions to add a space between overloaded operator and opening parentheses in clang-format.

[MyDeveloperDay]

you need to add a unit test int clang/unittest/Format/FormatTest.cpp (we tend to not use lit tests)

[HazardyKnusperkeks]

clang/include/clang/Format/Format.h

3452

Please put the vs. on the same column as the others.

3455

Please sort alphabetical.

clang/lib/Format/Format.cpp

874

Sort

[rajatbajpai]

Incorporated review comments.

Changelog:

1. Using unit test case instead of lit test case.
2. Placed the option according to alphabetical order.

[rajatbajpai]

Fixed the patch file.

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

14539

There should be a PARSE unit test too please

14540

[curdeius]

clang/docs/ClangFormatStyleOptions.rst

3821

I'm not fond of the current name, exactly the "Overloading" part (but have no better suggestion right now). It seems a bit misleading to me.

Maybe AfterOverloadedOperator(Name)??? At least it would match the token kind name.

[MyDeveloperDay]

clang/docs/ClangFormatStyleOptions.rst

3821

+1 here, AfterOperator?

[HazardyKnusperkeks]

Does it affect calling code? a.operator++(5); Should it? But please add tests for that.

[rajatbajpai]

In D116283#3211590, @HazardyKnusperkeks wrote:

Does it affect calling code? a.operator++(5); Should it? But please add tests for that.

Yes, it does affect the calling code as well. However, I am not sure if we should separate the two. Sure, I'll add a test case for it.

clang/docs/ClangFormatStyleOptions.rst

3821

@MyDeveloperDay Initially, I thought about using AfterOperator but it also seems a bit misleading. However, @curdeius suggestion seems reasonable given it matches with the token kind.

[rajatbajpai]

clang/unittests/Format/FormatTest.cpp

14539

I'm sorry if I misunderstood it, do you mean tests using CHECK_PARSE? I am confirming this because I didn't find any such test case for existing SpaceBeforeParensOptions options.

[HazardyKnusperkeks]

clang/unittests/Format/FormatTest.cpp

14539

Than this has slipped through, but there should be a test for every parsing aspect.

[rajatbajpai]

clang/unittests/Format/FormatTest.cpp

14539

Thanks for confirming.

Not an issue, I'll try to add a test case for existing options as well then.

[rajatbajpai]

Incorporated review comments.

1. Added Check Parse test case for SpaceBeforeParensOptions.
2. Renamed the option to AfterOverloadedOperator.
3. Added operator overloading instantiation scenario in the unit test as well.

[MyDeveloperDay]

LGTM, could you add a release note into docs/ReleaseNotes.rst?

[curdeius]

clang/docs/ClangFormatStyleOptions.rst

3826

Please add operator call as well to the example (object.operator++ (0)).

[rajatbajpai]

In D116283#3216235, @MyDeveloperDay wrote:

LGTM, could you add a release note into docs/ReleaseNotes.rst?

Sure, will do.

clang/docs/ClangFormatStyleOptions.rst

3826

Sure, will add.

[rajatbajpai]

Incorporated review comments.

Updated release note and example scenario.

[curdeius]

LGTM.
If you need help landing this (if you don't have commit rights), just give your name and email that you want to be used for the contribution and someone will land it for you.

See also https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access if you think contributing more patches.

[HazardyKnusperkeks]

Thanks for adding the parse checks.

[rajatbajpai]

In D116283#3218118, @HazardyKnusperkeks wrote:

Thanks for adding the parse checks.

Not an issue :)

In D116283#3218101, @curdeius wrote:

LGTM.
If you need help landing this (if you don't have commit rights), just give your name and email that you want to be used for the contribution and someone will land it for you.

See also https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access if you think contributing more patches.

Thanks everyone for taking time to review this change.

Yes, I don't have commit rights. Please land this change for me (name : Rajat Bajpai and email: [e-mail]).

I will apply for commit access for future changes.

clang/docs/ClangFormatStyleOptions.rst

**SpaceBeforeParensOptions** (``SpaceBeforeParensCustom``) :versionbadge:`clang-format 14`
   * ``bool AfterIfMacros`` If ``true``, put space between if macros and opening parentheses.
 
     .. code-block:: c++
 
        true:                                  false:
        IF (...)                        vs.    IF(...)
          <conditional-body>                     <conditional-body>
 
+  * ``bool AfterOverloadedOperator`` If ``true``, put a space between operator overloading and opening
+    parentheses.
+
+    .. code-block:: c++
+
+       true:                                  false:
+       void operator++ (int a);        vs.    void operator++(int a);
+       object.operator++ (10);                object.operator++(10);
+
   * ``bool BeforeNonEmptyParentheses`` If ``true``, put a space before opening parentheses only if the
     parentheses are not empty.
 
     .. code-block:: c++
 
        true:                                  false:
        void f (int a);                 vs.    void f();
        f (a);                                 f();

clang/docs/ReleaseNotes.rst

- Add a ``Custom`` style to ``SpaceBeforeParens``, to better configure the
   ``SpaceBeforeParensOptions``.
 
 - The command line argument `-style=<string>` has been extended so that a specific
   format file at location <format_file_path> can be selected. This is supported
   via the syntax: `-style=file:<format_file_path>`.
 
 - Improved C++20 Modules and Coroutines support.
 
+- Option ``AfterOverloadedOperator`` has been added in ``SpaceBeforeParensOptions``
+  to allow space between overloaded operator and opening parentheses.
+
 libclang
 --------
 
 - ...
 
 Static Analyzer
 ---------------
 

clang/include/clang/Format/Format.h

struct SpaceBeforeParensCustom {
     bool AfterFunctionDefinitionName;
     /// If ``true``, put space between if macros and opening parentheses.
     /// \code
     ///    true:                                  false:
     ///    IF (...)                        vs.    IF(...)
     ///      <conditional-body>                     <conditional-body>
     /// \endcode
     bool AfterIfMacros;
+    /// If ``true``, put a space between operator overloading and opening
+    /// parentheses.
+    /// \code
+    ///    true:                                  false:
+    ///    void operator++ (int a);        vs.    void operator++(int a);
+    ///    object.operator++ (10);                object.operator++(10);
+    /// \endcode
+    bool AfterOverloadedOperator;
     /// If ``true``, put a space before opening parentheses only if the
     /// parentheses are not empty.
     /// \code
     ///    true:                                  false:
     ///    void f (int a);                 vs.    void f();
     ///    f (a);                                 f();
     /// \endcode
     bool BeforeNonEmptyParentheses;
 
     SpaceBeforeParensCustom()
         : AfterControlStatements(false), AfterForeachMacros(false),
           AfterFunctionDeclarationName(false),
           AfterFunctionDefinitionName(false), AfterIfMacros(false),
-          BeforeNonEmptyParentheses(false) {}
+          AfterOverloadedOperator(false), BeforeNonEmptyParentheses(false) {}
 
     bool operator==(const SpaceBeforeParensCustom &Other) const {
       return AfterControlStatements == Other.AfterControlStatements &&
              AfterForeachMacros == Other.AfterForeachMacros &&
              AfterFunctionDeclarationName ==
                  Other.AfterFunctionDeclarationName &&
              AfterFunctionDefinitionName == Other.AfterFunctionDefinitionName &&
              AfterIfMacros == Other.AfterIfMacros &&
+             AfterOverloadedOperator == Other.AfterOverloadedOperator &&
              BeforeNonEmptyParentheses == Other.BeforeNonEmptyParentheses;
     }
   };
 
   /// Control of individual space before parentheses.
   ///
   /// If ``SpaceBeforeParens`` is set to ``Custom``, use this to specify
   /// how each individual space before parentheses case should be handled.

clang/lib/Format/Format.cpp

template <> struct MappingTraits<FormatStyle::SpaceBeforeParensCustom> {
   static void mapping(IO &IO, FormatStyle::SpaceBeforeParensCustom &Spacing) {
     IO.mapOptional("AfterControlStatements", Spacing.AfterControlStatements);
     IO.mapOptional("AfterForeachMacros", Spacing.AfterForeachMacros);
     IO.mapOptional("AfterFunctionDefinitionName",
                    Spacing.AfterFunctionDefinitionName);
     IO.mapOptional("AfterFunctionDeclarationName",
                    Spacing.AfterFunctionDeclarationName);
     IO.mapOptional("AfterIfMacros", Spacing.AfterIfMacros);
+    IO.mapOptional("AfterOverloadedOperator", Spacing.AfterOverloadedOperator);
     IO.mapOptional("BeforeNonEmptyParentheses",
                    Spacing.BeforeNonEmptyParentheses);
   }
 };
 
 template <> struct MappingTraits<FormatStyle::RawStringFormat> {
   static void mapping(IO &IO, FormatStyle::RawStringFormat &Format) {
     IO.mapOptional("Language", Format.Language);
     IO.mapOptional("Delimiters", Format.Delimiters);
     IO.mapOptional("EnclosingFunctions", Format.EnclosingFunctions);
     IO.mapOptional("CanonicalDelimiter", Format.CanonicalDelimiter);

clang/lib/Format/TokenAnnotator.cpp

if (Level == prec::Assignment)
     return Style.PenaltyBreakAssignment;
   if (Level != prec::Unknown)
     return Level;
 
   return 3;
 }
 
 bool TokenAnnotator::spaceRequiredBeforeParens(const FormatToken &Right) const {
-  return Style.SpaceBeforeParens == FormatStyle::SBPO_Always ||
-         (Style.SpaceBeforeParensOptions.BeforeNonEmptyParentheses &&
-          Right.ParameterCount > 0);
+  if (Style.SpaceBeforeParens == FormatStyle::SBPO_Always)
+    return true;
+  if (Right.is(TT_OverloadedOperatorLParen) &&
+      Style.SpaceBeforeParensOptions.AfterOverloadedOperator)
+    return true;
+  if (Style.SpaceBeforeParensOptions.BeforeNonEmptyParentheses &&
+      Right.ParameterCount > 0)
+    return true;
+  return false;
 }
 
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
                                           const FormatToken &Left,
                                           const FormatToken &Right) {
   if (Left.is(tok::kw_return) && Right.isNot(tok::semi))
     return true;
   if (Style.isJson() && Left.is(tok::string_literal) && Right.is(tok::colon))

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ConfigurableSpaceBeforeParens) {
   verifyFormat("static_assert (sizeof (char) == 1, \"Impossible!\");",
                SomeSpace2);
   verifyFormat("int f() throw (Deprecated);", SomeSpace2);
   verifyFormat("typedef void (*cb) (int);", SomeSpace2);
   verifyFormat("T A::operator()();", SomeSpace2);
   // verifyFormat("X A::operator++ (T);", SomeSpace2);
   verifyFormat("int x = int (y);", SomeSpace2);
   verifyFormat("auto lambda = []() { return 0; };", SomeSpace2);
+
+  FormatStyle SpaceAfterOverloadedOperator = getLLVMStyle();
+  SpaceAfterOverloadedOperator.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  SpaceAfterOverloadedOperator.SpaceBeforeParensOptions
+      .AfterOverloadedOperator = true;
+
+  verifyFormat("auto operator++ () -> int;", SpaceAfterOverloadedOperator);
+  verifyFormat("X A::operator++ ();", SpaceAfterOverloadedOperator);
+  verifyFormat("some_object.operator++ ();", SpaceAfterOverloadedOperator);
+  verifyFormat("auto func() -> int;", SpaceAfterOverloadedOperator);
+
+  SpaceAfterOverloadedOperator.SpaceBeforeParensOptions
+      .AfterOverloadedOperator = false;
+
+  verifyFormat("auto operator++() -> int;", SpaceAfterOverloadedOperator);
+  verifyFormat("X A::operator++();", SpaceAfterOverloadedOperator);
+  verifyFormat("some_object.operator++();", SpaceAfterOverloadedOperator);
+  verifyFormat("auto func() -> int;", SpaceAfterOverloadedOperator);
 }
 
 TEST_F(FormatTest, SpaceAfterLogicalNot) {
   FormatStyle Spaces = getLLVMStyle();
   Spaces.SpaceAfterLogicalNot = true;
 
   verifyFormat("bool x = ! y", Spaces);
   verifyFormat("if (! isFailure())", Spaces);
TEST_F(FormatTest, ParsesConfigurationBools) {
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, BeforeCatch);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, BeforeElse);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, BeforeLambdaBody);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, BeforeWhile);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, IndentBraces);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, SplitEmptyFunction);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, SplitEmptyRecord);
   CHECK_PARSE_NESTED_BOOL(BraceWrapping, SplitEmptyNamespace);
+  CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions, AfterControlStatements);
+  CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions, AfterForeachMacros);
+  CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions,
+                          AfterFunctionDeclarationName);
+  CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions,
+                          AfterFunctionDefinitionName);
+  CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions, AfterIfMacros);
+  CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions, AfterOverloadedOperator);
+  CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions, BeforeNonEmptyParentheses);
 }
 
 #undef CHECK_PARSE_BOOL
 
 TEST_F(FormatTest, ParsesConfiguration) {
   FormatStyle Style = {};
   Style.Language = FormatStyle::LK_Cpp;
   CHECK_PARSE("AccessModifierOffset: -1234", AccessModifierOffset, -1234);