[clang-format] Simplify the AfterPlacementOperator option #79796

owenca · 2024-01-29T08:51:26Z

Change AfterPlacementOperator to a boolean and deprecate SBPO_Never, which meant never inserting a space except when after new/delete.

Fixes #78892.

llvmbot · 2024-01-29T08:51:55Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-format

Author: Owen Pan (owenca)

Changes

Change AfterPlacementOperator to a boolean. Also add SBPO_None for never inserting a space before a left parenthesis and deprecate SBPO_Never, which meant never inserting a space except when after new/delete.

Fixes #78892.

Full diff: https://github.com/llvm/llvm-project/pull/79796.diff

6 Files Affected:

(modified) clang/docs/ClangFormatStyleOptions.rst (+12-25)
(modified) clang/include/clang/Format/Format.h (+13-23)
(modified) clang/lib/Format/Format.cpp (+6-22)
(modified) clang/lib/Format/TokenAnnotator.cpp (+1-8)
(modified) clang/unittests/Format/ConfigParseTest.cpp (+3-18)
(modified) clang/unittests/Format/FormatTest.cpp (+14-18)

diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index 4dc0de3a90f2650..ce4bf867b26195f 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -5276,7 +5276,7 @@ the configuration (without a prefix: ``Auto``).
 
   Possible values:
 
-  * ``SBPO_Never`` (in configuration: ``Never``)
+  * ``SBPO_None`` (in configuration: ``None``)
     Never put a space before opening parentheses.
 
     .. code-block:: c++
@@ -5287,6 +5287,11 @@ the configuration (without a prefix: ``Auto``).
          }
        }
 
+  * ``SBPO_Never`` (in configuration: ``Never``)
+    This is **deprecated** and replaced by ``Custom`` below, with all
+    ``SpaceBeforeParensOptions`` but ``AfterPlacementOperator`` set to
+    ``false``.
+
   * ``SBPO_ControlStatements`` (in configuration: ``ControlStatements``)
     Put a space before opening parentheses only after control statement
     keywords (``for/if/while...``).
@@ -5425,32 +5430,14 @@ the configuration (without a prefix: ``Auto``).
        void operator++ (int a);        vs.    void operator++(int a);
        object.operator++ (10);                object.operator++(10);
 
-  * ``AfterPlacementOperatorStyle AfterPlacementOperator`` :versionbadge:`clang-format 18`
-
-    Defines in which cases to put a space between ``new/delete`` operators
-    and opening parentheses.
+  * ``bool AfterPlacementOperator`` If ``true``, put a space between operator ``new``/``delete`` and opening
+    parenthesis.
 
-    Possible values:
-
-    * ``APO_Never`` (in configuration: ``Never``)
-      Remove space after ``new/delete`` operators and before ``(``.
-
-      .. code-block:: c++
-
-         new(buf) T;
-         delete(buf) T;
-
-    * ``APO_Always`` (in configuration: ``Always``)
-      Always add space after ``new/delete`` operators and before ``(``.
-
-      .. code-block:: c++
-
-         new (buf) T;
-         delete (buf) T;
-
-    * ``APO_Leave`` (in configuration: ``Leave``)
-      Leave placement ``new/delete`` expressions as they are.
+    .. code-block:: c++
 
+       true:                                  false:
+       new (buf) T;                    vs.    new(buf) T;
+       delete (buf) T;                        delete(buf) T;
 
   * ``bool AfterRequiresInClause`` If ``true``, put space between requires keyword in a requires clause and
     opening parentheses, if there is one.
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index bc9eecd42f9ebfd..5c536bc3f381fab 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -4165,6 +4165,10 @@ struct FormatStyle {
     ///      }
     ///    }
     /// \endcode
+    SBPO_None,
+    /// This is **deprecated** and replaced by ``Custom`` below, with all
+    /// ``SpaceBeforeParensOptions`` but ``AfterPlacementOperator`` set to
+    /// ``false``.
     SBPO_Never,
     /// Put a space before opening parentheses only after control statement
     /// keywords (``for/if/while...``).
@@ -4273,28 +4277,14 @@ struct FormatStyle {
     ///    object.operator++ (10);                object.operator++(10);
     /// \endcode
     bool AfterOverloadedOperator;
-    /// Styles for adding spacing between ``new/delete`` operators and opening
-    /// parentheses.
-    enum AfterPlacementOperatorStyle : int8_t {
-      /// Remove space after ``new/delete`` operators and before ``(``.
-      /// \code
-      ///    new(buf) T;
-      ///    delete(buf) T;
-      /// \endcode
-      APO_Never,
-      /// Always add space after ``new/delete`` operators and before ``(``.
-      /// \code
-      ///    new (buf) T;
-      ///    delete (buf) T;
-      /// \endcode
-      APO_Always,
-      /// Leave placement ``new/delete`` expressions as they are.
-      APO_Leave,
-    };
-    /// Defines in which cases to put a space between ``new/delete`` operators
-    /// and opening parentheses.
-    /// \version 18
-    AfterPlacementOperatorStyle AfterPlacementOperator;
+    /// If ``true``, put a space between operator ``new``/``delete`` and opening
+    /// parenthesis.
+    /// \code
+    ///    true:                                  false:
+    ///    new (buf) T;                    vs.    new(buf) T;
+    ///    delete (buf) T;                        delete(buf) T;
+    /// \endcode
+    bool AfterPlacementOperator;
     /// If ``true``, put space between requires keyword in a requires clause and
     /// opening parentheses, if there is one.
     /// \code
@@ -4327,7 +4317,7 @@ struct FormatStyle {
         : AfterControlStatements(false), AfterForeachMacros(false),
           AfterFunctionDeclarationName(false),
           AfterFunctionDefinitionName(false), AfterIfMacros(false),
-          AfterOverloadedOperator(false), AfterPlacementOperator(APO_Leave),
+          AfterOverloadedOperator(false), AfterPlacementOperator(true),
           AfterRequiresInClause(false), AfterRequiresInExpression(false),
           BeforeNonEmptyParentheses(false) {}
 
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index ff326dc784783b2..adf07b8225e7c65 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -504,22 +504,6 @@ struct ScalarEnumerationTraits<FormatStyle::QualifierAlignmentStyle> {
   }
 };
 
-template <>
-struct MappingTraits<
-    FormatStyle::SpaceBeforeParensCustom::AfterPlacementOperatorStyle> {
-  static void
-  mapping(IO &IO,
-          FormatStyle::SpaceBeforeParensCustom::AfterPlacementOperatorStyle
-              &Value) {
-    IO.enumCase(Value, "Always",
-                FormatStyle::SpaceBeforeParensCustom::APO_Always);
-    IO.enumCase(Value, "Never",
-                FormatStyle::SpaceBeforeParensCustom::APO_Never);
-    IO.enumCase(Value, "Leave",
-                FormatStyle::SpaceBeforeParensCustom::APO_Leave);
-  }
-};
-
 template <> struct MappingTraits<FormatStyle::RawStringFormat> {
   static void mapping(IO &IO, FormatStyle::RawStringFormat &Format) {
     IO.mapOptional("Language", Format.Language);
@@ -707,6 +691,7 @@ template <> struct MappingTraits<FormatStyle::SpaceBeforeParensCustom> {
 template <>
 struct ScalarEnumerationTraits<FormatStyle::SpaceBeforeParensStyle> {
   static void enumeration(IO &IO, FormatStyle::SpaceBeforeParensStyle &Value) {
+    IO.enumCase(Value, "None", FormatStyle::SBPO_None);
     IO.enumCase(Value, "Never", FormatStyle::SBPO_Never);
     IO.enumCase(Value, "ControlStatements",
                 FormatStyle::SBPO_ControlStatements);
@@ -1389,11 +1374,12 @@ static void expandPresetsSpaceBeforeParens(FormatStyle &Expanded) {
   // Reset all flags
   Expanded.SpaceBeforeParensOptions = {};
 
+  if (Expanded.SpaceBeforeParens == FormatStyle::SBPO_None)
+    return;
+
+  Expanded.SpaceBeforeParensOptions.AfterPlacementOperator = true;
+
   switch (Expanded.SpaceBeforeParens) {
-  case FormatStyle::SBPO_Never:
-    Expanded.SpaceBeforeParensOptions.AfterPlacementOperator =
-        FormatStyle::SpaceBeforeParensCustom::APO_Never;
-    break;
   case FormatStyle::SBPO_ControlStatements:
     Expanded.SpaceBeforeParensOptions.AfterControlStatements = true;
     Expanded.SpaceBeforeParensOptions.AfterForeachMacros = true;
@@ -1405,8 +1391,6 @@ static void expandPresetsSpaceBeforeParens(FormatStyle &Expanded) {
   case FormatStyle::SBPO_NonEmptyParentheses:
     Expanded.SpaceBeforeParensOptions.BeforeNonEmptyParentheses = true;
     break;
-  case FormatStyle::SBPO_Always:
-    break;
   default:
     break;
   }
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index df1c5bc19de1e84..d0c4273cfc7e583 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -4274,14 +4274,7 @@ bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
         Left.isOneOf(tok::kw_new, tok::kw_delete) &&
         Right.isNot(TT_OverloadedOperatorLParen) &&
         !(Line.MightBeFunctionDecl && Left.is(TT_FunctionDeclarationName))) {
-      if (Style.SpaceBeforeParensOptions.AfterPlacementOperator ==
-              FormatStyle::SpaceBeforeParensCustom::APO_Always ||
-          (Style.SpaceBeforeParensOptions.AfterPlacementOperator ==
-               FormatStyle::SpaceBeforeParensCustom::APO_Leave &&
-           Right.hasWhitespaceBefore())) {
-        return true;
-      }
-      return false;
+      return Style.SpaceBeforeParensOptions.AfterPlacementOperator;
     }
     if (Line.Type == LT_ObjCDecl)
       return true;
diff --git a/clang/unittests/Format/ConfigParseTest.cpp b/clang/unittests/Format/ConfigParseTest.cpp
index 2a8d79359a49b40..d92e373d24a871c 100644
--- a/clang/unittests/Format/ConfigParseTest.cpp
+++ b/clang/unittests/Format/ConfigParseTest.cpp
@@ -231,6 +231,7 @@ TEST(ConfigParseTest, ParsesConfigurationBools) {
                           AfterFunctionDefinitionName);
   CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions, AfterIfMacros);
   CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions, AfterOverloadedOperator);
+  CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions, AfterPlacementOperator);
   CHECK_PARSE_NESTED_BOOL(SpaceBeforeParensOptions, BeforeNonEmptyParentheses);
   CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, InCStyleCasts);
   CHECK_PARSE_NESTED_BOOL(SpacesInParensOptions, InConditionalStatements);
@@ -587,6 +588,8 @@ TEST(ConfigParseTest, ParsesConfiguration) {
               SpaceAroundPointerQualifiers, FormatStyle::SAPQ_Both);
 
   Style.SpaceBeforeParens = FormatStyle::SBPO_Always;
+  CHECK_PARSE("SpaceBeforeParens: None", SpaceBeforeParens,
+              FormatStyle::SBPO_None);
   CHECK_PARSE("SpaceBeforeParens: Never", SpaceBeforeParens,
               FormatStyle::SBPO_Never);
   CHECK_PARSE("SpaceBeforeParens: Always", SpaceBeforeParens,
@@ -609,24 +612,6 @@ TEST(ConfigParseTest, ParsesConfiguration) {
               SpaceBeforeParens,
               FormatStyle::SBPO_ControlStatementsExceptControlMacros);
 
-  Style.SpaceBeforeParens = FormatStyle::SBPO_Custom;
-  Style.SpaceBeforeParensOptions.AfterPlacementOperator =
-      FormatStyle::SpaceBeforeParensCustom::APO_Always;
-  CHECK_PARSE("SpaceBeforeParensOptions:\n"
-              "  AfterPlacementOperator: Never",
-              SpaceBeforeParensOptions.AfterPlacementOperator,
-              FormatStyle::SpaceBeforeParensCustom::APO_Never);
-
-  CHECK_PARSE("SpaceBeforeParensOptions:\n"
-              "  AfterPlacementOperator: Always",
-              SpaceBeforeParensOptions.AfterPlacementOperator,
-              FormatStyle::SpaceBeforeParensCustom::APO_Always);
-
-  CHECK_PARSE("SpaceBeforeParensOptions:\n"
-              "  AfterPlacementOperator: Leave",
-              SpaceBeforeParensOptions.AfterPlacementOperator,
-              FormatStyle::SpaceBeforeParensCustom::APO_Leave);
-
   // For backward compatibility:
   Style.SpacesInParens = FormatStyle::SIPO_Never;
   Style.SpacesInParensOptions = {};
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index e5e763edf5b5bf6..a471e36f8d6825f 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -11347,35 +11347,31 @@ TEST_F(FormatTest, UnderstandsNewAndDelete) {
 
   FormatStyle AfterPlacementOperator = getLLVMStyle();
   AfterPlacementOperator.SpaceBeforeParens = FormatStyle::SBPO_Custom;
-  EXPECT_EQ(
-      AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator,
-      FormatStyle::SpaceBeforeParensCustom::APO_Leave);
+  EXPECT_TRUE(
+      AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator);
   verifyFormat("new (buf) int;", AfterPlacementOperator);
-  verifyFormat("new(buf) int;", AfterPlacementOperator);
-
-  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator =
-      FormatStyle::SpaceBeforeParensCustom::APO_Never;
   verifyFormat("struct A {\n"
                "  int *a;\n"
-               "  A(int *p) : a(new(p) int) {\n"
-               "    new(p) int;\n"
-               "    int *b = new(p) int;\n"
-               "    int *c = new(p) int(3);\n"
-               "    delete(b);\n"
+               "  A(int *p) : a(new (p) int) {\n"
+               "    new (p) int;\n"
+               "    int *b = new (p) int;\n"
+               "    int *c = new (p) int(3);\n"
+               "    delete (b);\n"
                "  }\n"
                "};",
                AfterPlacementOperator);
   verifyFormat("void operator new(void *foo) ATTRIB;", AfterPlacementOperator);
 
   AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator =
-      FormatStyle::SpaceBeforeParensCustom::APO_Always;
+      false;
+  verifyFormat("new(buf) int;", AfterPlacementOperator);
   verifyFormat("struct A {\n"
                "  int *a;\n"
-               "  A(int *p) : a(new (p) int) {\n"
-               "    new (p) int;\n"
-               "    int *b = new (p) int;\n"
-               "    int *c = new (p) int(3);\n"
-               "    delete (b);\n"
+               "  A(int *p) : a(new(p) int) {\n"
+               "    new(p) int;\n"
+               "    int *b = new(p) int;\n"
+               "    int *c = new(p) int(3);\n"
+               "    delete(b);\n"
                "  }\n"
                "};",
                AfterPlacementOperator);

Change AfterPlacementOperator to a boolean. Also add SBPO_None for never inserting a space before a left parenthesis and deprecate SBPO_Never, which meant never inserting a space except when after new/delete. Fixes llvm#78892.

Don't map Never to SBPO_Custom as SBPO_Never goes through expandPresetsSpaceBeforeParens() but SBPO_Custom doesn't.

Change AfterPlacementOperator to a boolean and deprecate SBPO_Never, which meant never inserting a space except when after new/delete. Fixes llvm#78892. (cherry picked from commit 908fd09)

Change AfterPlacementOperator to a boolean and deprecate SBPO_Never, which meant never inserting a space except when after new/delete. Fixes llvm#78892.

Change AfterPlacementOperator to a boolean and deprecate SBPO_Never, which meant never inserting a space except when after new/delete. Fixes llvm#78892. (cherry picked from commit 908fd09)

llvmbot added clang Clang issues not falling into any other category clang-format labels Jan 29, 2024

owenca removed the clang Clang issues not falling into any other category label Jan 29, 2024

[clang-format] Simplify the AfterPlacementOperator option

d40f2ed

Change AfterPlacementOperator to a boolean. Also add SBPO_None for never inserting a space before a left parenthesis and deprecate SBPO_Never, which meant never inserting a space except when after new/delete. Fixes llvm#78892.

owenca force-pushed the 78892 branch from 46f4a74 to d40f2ed Compare January 29, 2024 16:56

llvmbot added the clang Clang issues not falling into any other category label Jan 29, 2024

HazardyKnusperkeks approved these changes Jan 29, 2024

View reviewed changes

Don't add SBPO_None.

3c11c03

Don't map Never to SBPO_Custom as SBPO_Never goes through expandPresetsSpaceBeforeParens() but SBPO_Custom doesn't.

owenca requested a review from HazardyKnusperkeks January 31, 2024 06:41

owenca removed the clang Clang issues not falling into any other category label Feb 1, 2024

owenca merged commit 908fd09 into llvm:main Feb 1, 2024
5 checks passed

owenca deleted the 78892 branch February 1, 2024 04:20

pointhex mentioned this pull request May 7, 2024

getStyleDiagHandler #91314

Closed

aemerson mentioned this pull request May 9, 2024

release/18.x: [AArc64][GlobalISel] Fix legalizer assert for G_INSERT_VECTOR_ELT - manual merge #91672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang-format] Simplify the AfterPlacementOperator option #79796

[clang-format] Simplify the AfterPlacementOperator option #79796

owenca commented Jan 29, 2024 •

edited

Loading

llvmbot commented Jan 29, 2024 •

edited

Loading

[clang-format] Simplify the AfterPlacementOperator option #79796

[clang-format] Simplify the AfterPlacementOperator option #79796

Conversation

owenca commented Jan 29, 2024 • edited Loading

llvmbot commented Jan 29, 2024 • edited Loading

owenca commented Jan 29, 2024 •

edited

Loading

llvmbot commented Jan 29, 2024 •

edited

Loading