[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

omarahmed updated this revision to Diff 445172.
omarahmed added a comment.

Change removes to remove


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127270

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/tools/dump_format_style.py
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -10130,6 +10130,42 @@
 "void delete(link p);\n",
 format("void new (link p);\n"
"void delete (link p);\n"));
+
+  FormatStyle AfterPlacementOperator = getLLVMStyle();
+  AfterPlacementOperator.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  EXPECT_EQ(
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator,
+  FormatStyle::SpaceBeforeParensCustom::APO_Leave);
+  EXPECT_EQ("new (buf) int;", format("new (buf) int;", AfterPlacementOperator));
+  EXPECT_EQ("new(buf) int;", format("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"
+   "  }\n"
+   "};",
+   AfterPlacementOperator);
+  verifyFormat("void operator new(void *foo) ATTRIB;", AfterPlacementOperator);
+
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator =
+  FormatStyle::SpaceBeforeParensCustom::APO_Always;
+  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"
+   "  }\n"
+   "};",
+   AfterPlacementOperator);
+  verifyFormat("void operator new(void *foo) ATTRIB;", AfterPlacementOperator);
 }
 
 TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
@@ -20308,6 +20344,24 @@
   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);
+
   Style.ColumnLimit = 123;
   FormatStyle BaseStyle = getLLVMStyle();
   CHECK_PARSE("BasedOnStyle: LLVM", ColumnLimit, BaseStyle.ColumnLimit);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3393,6 +3393,18 @@
 if (Left.is(TT_IfMacro))
   return Style.SpaceBeforeParensOptions.AfterIfMacros ||
  spaceRequiredBeforeParens(Right);
+if (Style.SpaceBeforeParens == FormatStyle::SBPO_Custom &&
+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;
+}
 if (Line.Type == LT_ObjCDecl)
   return true;
 if (Left.is(tok::semi))
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -936,6 +936,7 @@
Spacing.AfterFunctionDeclarationName);
 IO.mapOptional("AfterIfMacros", Spacing.AfterIfMacros);
 

[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

omarahmed added a comment.

I don't have push credentials so If everything is okay with the patch, can you 
push it for me. My email is omarpiratee2...@gmail.com




Comment at: clang/include/clang/Format/Format.h:3555
   AfterFunctionDefinitionName(false), AfterIfMacros(false),
-  AfterOverloadedOperator(false), AfterRequiresInClause(false),
-  AfterRequiresInExpression(false), BeforeNonEmptyParentheses(false) {}
+  AfterOverloadedOperator(false), AfterPlacementOperator(APO_Leave),
+  AfterRequiresInClause(false), AfterRequiresInExpression(false),

HazardyKnusperkeks wrote:
> We change the behavior here, I'm still not convinced we should do that. 
> Although I'm a big supporter of the `Leave` option.
I am neutral to both cases, so I leave the choice to you as I am not 
experienced with clang-format users. Let me know which choice you think would 
be better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127270

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

omarahmed updated this revision to Diff 438962.
omarahmed added a comment.

Format files


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127270

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/tools/dump_format_style.py
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -10130,6 +10130,42 @@
 "void delete(link p);\n",
 format("void new (link p);\n"
"void delete (link p);\n"));
+
+  FormatStyle AfterPlacementOperator = getLLVMStyle();
+  AfterPlacementOperator.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  EXPECT_EQ(
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator,
+  FormatStyle::SpaceBeforeParensCustom::APO_Leave);
+  EXPECT_EQ("new (buf) int;", format("new (buf) int;", AfterPlacementOperator));
+  EXPECT_EQ("new(buf) int;", format("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"
+   "  }\n"
+   "};",
+   AfterPlacementOperator);
+  verifyFormat("void operator new(void *foo) ATTRIB;", AfterPlacementOperator);
+
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator =
+  FormatStyle::SpaceBeforeParensCustom::APO_Always;
+  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"
+   "  }\n"
+   "};",
+   AfterPlacementOperator);
+  verifyFormat("void operator new(void *foo) ATTRIB;", AfterPlacementOperator);
 }
 
 TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
@@ -20308,6 +20344,24 @@
   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);
+
   Style.ColumnLimit = 123;
   FormatStyle BaseStyle = getLLVMStyle();
   CHECK_PARSE("BasedOnStyle: LLVM", ColumnLimit, BaseStyle.ColumnLimit);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3393,6 +3393,18 @@
 if (Left.is(TT_IfMacro))
   return Style.SpaceBeforeParensOptions.AfterIfMacros ||
  spaceRequiredBeforeParens(Right);
+if (Style.SpaceBeforeParens == FormatStyle::SBPO_Custom &&
+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;
+}
 if (Line.Type == LT_ObjCDecl)
   return true;
 if (Left.is(tok::semi))
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -936,6 +936,7 @@
Spacing.AfterFunctionDeclarationName);
 IO.mapOptional("AfterIfMacros", Spacing.AfterIfMacros);
 

[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

omarahmed updated this revision to Diff 438719.
omarahmed added a comment.

Format files


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127270

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/tools/dump_format_style.py
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -10130,6 +10130,42 @@
 "void delete(link p);\n",
 format("void new (link p);\n"
"void delete (link p);\n"));
+
+  FormatStyle AfterPlacementOperator = getLLVMStyle();
+  AfterPlacementOperator.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  EXPECT_EQ(
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator,
+  FormatStyle::SpaceBeforeParensCustom::APO_Leave);
+  EXPECT_EQ("new (buf) int;", format("new (buf) int;", AfterPlacementOperator));
+  EXPECT_EQ("new(buf) int;", format("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"
+   "  }\n"
+   "};",
+   AfterPlacementOperator);
+  verifyFormat("void operator new(void *foo) ATTRIB;", AfterPlacementOperator);
+
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator =
+  FormatStyle::SpaceBeforeParensCustom::APO_Always;
+  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"
+   "  }\n"
+   "};",
+   AfterPlacementOperator);
+  verifyFormat("void operator new(void *foo) ATTRIB;", AfterPlacementOperator);
 }
 
 TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
@@ -20308,6 +20344,24 @@
   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);
+
   Style.ColumnLimit = 123;
   FormatStyle BaseStyle = getLLVMStyle();
   CHECK_PARSE("BasedOnStyle: LLVM", ColumnLimit, BaseStyle.ColumnLimit);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3393,6 +3393,18 @@
 if (Left.is(TT_IfMacro))
   return Style.SpaceBeforeParensOptions.AfterIfMacros ||
  spaceRequiredBeforeParens(Right);
+if (Style.SpaceBeforeParens == FormatStyle::SBPO_Custom &&
+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;
+}
 if (Line.Type == LT_ObjCDecl)
   return true;
 if (Left.is(tok::semi))
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -936,6 +936,7 @@
Spacing.AfterFunctionDeclarationName);
 IO.mapOptional("AfterIfMacros", Spacing.AfterIfMacros);
 

[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

omarahmed added inline comments.



Comment at: clang/include/clang/Format/Format.h:3559
   AfterFunctionDefinitionName(false), AfterIfMacros(false),
-  AfterOverloadedOperator(false), AfterRequiresInClause(false),
-  AfterRequiresInExpression(false), BeforeNonEmptyParentheses(false) {}
+  AfterOverloadedOperator(false), AfterPlacementOperator(APO_Always),
+  AfterRequiresInClause(false), AfterRequiresInExpression(false),

MyDeveloperDay wrote:
> HazardyKnusperkeks wrote:
> > As said in the other comment, there is code that depends on setting to 
> > never in the default constructor, so please do.
> Why Never and not Leave? basically introducing one or the other as a default 
> won't that causes changes for people who don't want to use this?
I think that's a good point. I changed that to `APO_Leave` and added 
`APO_Never` to the code that depends on it. 

I was guarding the default behavior with `SpaceBeforeParentheses: Custom` 
condition but I think still if someone defined it to be custom and didn't 
specify this option, it should still be leave.



Comment at: clang/lib/Format/Format.cpp:1169
   case FormatStyle::SBPO_Never:
+Expanded.SpaceBeforeParensOptions.AfterPlacementOperator = 
FormatStyle::SpaceBeforeParensCustom::APO_Never;
 break;

The part that depends on it to be `APO_Never` to align with `SBPO_Never` option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127270

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

omarahmed updated this revision to Diff 438657.
omarahmed added a comment.

Change the default to APO_Leave


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127270

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/tools/dump_format_style.py
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -10130,6 +10130,42 @@
 "void delete(link p);\n",
 format("void new (link p);\n"
"void delete (link p);\n"));
+
+  FormatStyle AfterPlacementOperator = getLLVMStyle();
+  AfterPlacementOperator.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  EXPECT_EQ(
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator,
+  FormatStyle::SpaceBeforeParensCustom::APO_Leave);
+  EXPECT_EQ("new (buf) int;", format("new (buf) int;", AfterPlacementOperator));
+  EXPECT_EQ("new(buf) int;", format("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"
+   "  }\n"
+   "};",
+   AfterPlacementOperator);
+  verifyFormat("void operator new(void *foo) ATTRIB;", AfterPlacementOperator);
+
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator =
+  FormatStyle::SpaceBeforeParensCustom::APO_Always;
+  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"
+   "  }\n"
+   "};",
+   AfterPlacementOperator);
+  verifyFormat("void operator new(void *foo) ATTRIB;", AfterPlacementOperator);
 }
 
 TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
@@ -20308,6 +20344,24 @@
   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);
+
   Style.ColumnLimit = 123;
   FormatStyle BaseStyle = getLLVMStyle();
   CHECK_PARSE("BasedOnStyle: LLVM", ColumnLimit, BaseStyle.ColumnLimit);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3393,6 +3393,18 @@
 if (Left.is(TT_IfMacro))
   return Style.SpaceBeforeParensOptions.AfterIfMacros ||
  spaceRequiredBeforeParens(Right);
+if (Style.SpaceBeforeParens == FormatStyle::SBPO_Custom &&
+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;
+}
 if (Line.Type == LT_ObjCDecl)
   return true;
 if (Left.is(tok::semi))
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -936,6 +936,7 @@
Spacing.AfterFunctionDeclarationName);
 IO.mapOptional("AfterIfMacros", Spacing.AfterIfMacros);
 

[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

omarahmed marked 3 inline comments as done and an inline comment as not done.
omarahmed added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:3396
  spaceRequiredBeforeParens(Right);
+if (Style.SpaceBeforeParens == FormatStyle::SBPO_Custom &&
+Left.isOneOf(tok::kw_new, tok::kw_delete) &&

HazardyKnusperkeks wrote:
> omarahmed wrote:
> > MyDeveloperDay wrote:
> > > shouldn't the very first part of this be? 
> > > 
> > > 
> > > ```
> > > if (Style.SpaceBeforeParensOptions.AfterPlacementOperator != 
> > > FormatStyle::SpaceBeforeParensCustom::APO_Leave)
> > > {
> > > 
> > > 
> > > 
> > > }
> > > ```
> > > 
> > > i.e. don't we want a zero change if someone says "Leave"
> > The current code logic, when we do not enter this suggested condition, will 
> > switch to [this 
> > condition](https://github.com/llvm/llvm-project/blob/c2bb2e5973acd24c45c1823e95e8e33003c59484/clang/lib/Format/TokenAnnotator.cpp#L3580).
> >  And will not leave the code as it is, but will change it.
> > 
> > I have thought of getting rid of this condition entirely, but it handles 
> > the default situation suggested here, not only for cpp but for other 
> > languages like js:
> > >>As I understand, the default behavior for when the user didn't use 
> > >>`SBPO_Custom` is to add a space in placement operators based on [this 
> > >>issue](https://github.com/llvm/llvm-project/issues/54703). And, at the 
> > >>same time, the default behavior should be `APO_Never` when we have 
> > >>`SBPO_Custom` so that we handle other code that depends on that. the 
> > >>existing tests confirm this understanding. So, the current logic was 
> > >>added based on this understanding.
> > 
> > I tried to add an extra condition to [this 
> > condition](https://github.com/llvm/llvm-project/blob/c2bb2e5973acd24c45c1823e95e8e33003c59484/clang/lib/Format/TokenAnnotator.cpp#L3580)
> >  to support the main logic that we are pursuing. so that it would be like 
> > that:
> > ```
> > if (Style.SpaceBeforeParensOptions.AfterPlacementOperator != 
> > FormatStyle::SpaceBeforeParensCustom::APO_Leave && 
> > Left.isOneOf(tok::kw_new, tok::kw_delete))
> > ```
> > But that wouldn't fix the leave situation, as it will be also at the end, 
> > reach [this 
> > part](https://github.com/llvm/llvm-project/blob/c2bb2e5973acd24c45c1823e95e8e33003c59484/clang/lib/Format/TokenAnnotator.cpp#L3598)
> >  which will  
> > ```
> > return false
> > ``` 
> > and force no space.
> > 
> > I think all of that was raised because all current 
> > "SpaceBeforeParanthesesOptions" are either true or false and this is the 
> > only enum
> > 
> > However, I completely agree with this logic which is to begin our checking 
> > for leave. But I think we should refactor all the 
> > "spaceBeforeParenthesesOptions" conditions to handle this and do that after 
> > all the options have been transformed to enums.
> > 
> `SpaceBeforeParensOptions` should always be expanded, regardless of 
> `SpaceBeforeParens`, as far as I remember. `SpaceBeforeParens` is only for 
> the one configuring clang-format, the code that formats should always and 
> only check `SpaceBeforeParensOptions`.
But in general, doesn't that could create some kind of conflict like if someone 
used a specific option in `SpaceBeforeParens ` and then defined inside 
`SpaceBeforeParensOptions ` some rules that could conflict with 
`SpaceBeforeParens `, what should be the priority between them?




Comment at: clang/lib/Format/TokenAnnotator.cpp:3396
  spaceRequiredBeforeParens(Right);
+if (Style.SpaceBeforeParens == FormatStyle::SBPO_Custom &&
+Left.isOneOf(tok::kw_new, tok::kw_delete) &&

omarahmed wrote:
> HazardyKnusperkeks wrote:
> > omarahmed wrote:
> > > MyDeveloperDay wrote:
> > > > shouldn't the very first part of this be? 
> > > > 
> > > > 
> > > > ```
> > > > if (Style.SpaceBeforeParensOptions.AfterPlacementOperator != 
> > > > FormatStyle::SpaceBeforeParensCustom::APO_Leave)
> > > > {
> > > > 
> > > > 
> > > > 
> > > > }
> > > > ```
> > > > 
> > > > i.e. don't we want a zero change if someone says "Leave"
> > > The current code logic, when we do not enter this suggested condition, 
> > > will switch to [this 
> > > condition](https://github.com/llvm/llvm-project/blob/c2bb2e5973acd24c45c1823e95e8e33003c59484/clang/lib/Format/TokenAnnotator.cpp#L3580).
> > >  And will not leave the code as it is, but will change it.
> > > 
> > > I have thought of getting rid of this condition entirely, but it handles 
> > > the default situation suggested here, not only for cpp but for other 
> > > languages like js:
> > > >>As I understand, the default behavior for when the user didn't use 
> > > >>`SBPO_Custom` is to add a space in placement operators based on [this 
> > > >>issue](https://github.com/llvm/llvm-project/issues/54703). And, at the 
> > > >>same time, the default behavior should be `APO_Never` when we 

[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

omarahmed added inline comments.



Comment at: clang/docs/tools/dump_format_style.py:317
   pass
+  elif state == State.InNestedEnum:
+if line.startswith('///'):

MyDeveloperDay wrote:
> Can you show us a screenshot of how these changes will look in HTML?
{F23524556}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127270

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

omarahmed updated this revision to Diff 438433.
omarahmed added a comment.

- Add version for nestedEnums and nestedFields
- Make tests valid


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127270

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/tools/dump_format_style.py
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -10130,6 +10130,42 @@
 "void delete(link p);\n",
 format("void new (link p);\n"
"void delete (link p);\n"));
+
+  FormatStyle AfterPlacementOperator = getLLVMStyle();
+  AfterPlacementOperator.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  EXPECT_EQ(
+  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"
+   "  }\n"
+   "};",
+   AfterPlacementOperator);
+  verifyFormat("void operator new(void *foo) ATTRIB;", AfterPlacementOperator);
+
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator =
+  FormatStyle::SpaceBeforeParensCustom::APO_Always;
+  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"
+   "  }\n"
+   "};",
+   AfterPlacementOperator);
+  verifyFormat("void operator new(void *foo) ATTRIB;", AfterPlacementOperator);
+
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator =
+  FormatStyle::SpaceBeforeParensCustom::APO_Leave;
+  EXPECT_EQ("new (buf) int;", format("new (buf) int;", AfterPlacementOperator));
+  EXPECT_EQ("new(buf) int;", format("new(buf) int;", AfterPlacementOperator));
 }
 
 TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
@@ -20308,6 +20344,24 @@
   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);
+
   Style.ColumnLimit = 123;
   FormatStyle BaseStyle = getLLVMStyle();
   CHECK_PARSE("BasedOnStyle: LLVM", ColumnLimit, BaseStyle.ColumnLimit);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3393,6 +3393,18 @@
 if (Left.is(TT_IfMacro))
   return Style.SpaceBeforeParensOptions.AfterIfMacros ||
  spaceRequiredBeforeParens(Right);
+if (Style.SpaceBeforeParens == FormatStyle::SBPO_Custom &&
+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;
+}
 if (Line.Type == LT_ObjCDecl)
   return true;
 if (Left.is(tok::semi))
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -936,6 +936,7 @@
Spacing.AfterFunctionDeclarationName);
 IO.mapOptional("AfterIfMacros", 

[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

omarahmed updated this revision to Diff 437980.
omarahmed added a comment.

Add Parse check


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127270

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/tools/dump_format_style.py
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -10130,6 +10130,42 @@
 "void delete(link p);\n",
 format("void new (link p);\n"
"void delete (link p);\n"));
+
+  FormatStyle AfterPlacementOperator = getLLVMStyle();
+  AfterPlacementOperator.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  EXPECT_EQ(
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator,
+  FormatStyle::SpaceBeforeParensCustom::APO_Never);
+  verifyFormat("struct A {\n"
+   "  void *a;\n"
+   "  A(void *p) : a(new(p) int) {\n"
+   "new(p) int;\n"
+   "int *b = new(p) int;\n"
+   "int *c = new(p) int(3);\n"
+   "int *d = delete(p) int(3);\n"
+   "  }\n"
+   "};",
+   AfterPlacementOperator);
+  verifyFormat("void operator new(void *foo) ATTRIB;", AfterPlacementOperator);
+
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator =
+  FormatStyle::SpaceBeforeParensCustom::APO_Always;
+  verifyFormat("struct A {\n"
+   "  void *a;\n"
+   "  A(void *p) : a(new (p) int) {\n"
+   "new (p) int;\n"
+   "int *b = new (p) int;\n"
+   "int *c = new (p) int(3);\n"
+   "int *d = delete (p) int(3);\n"
+   "  }\n"
+   "};",
+   AfterPlacementOperator);
+  verifyFormat("void operator new(void *foo) ATTRIB;", AfterPlacementOperator);
+
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator =
+  FormatStyle::SpaceBeforeParensCustom::APO_Leave;
+  EXPECT_EQ("new (buf) int;", format("new (buf) int;", AfterPlacementOperator));
+  EXPECT_EQ("new(buf) int;", format("new(buf) int;", AfterPlacementOperator));
 }
 
 TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
@@ -20308,6 +20344,24 @@
   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);
+
   Style.ColumnLimit = 123;
   FormatStyle BaseStyle = getLLVMStyle();
   CHECK_PARSE("BasedOnStyle: LLVM", ColumnLimit, BaseStyle.ColumnLimit);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3393,6 +3393,18 @@
 if (Left.is(TT_IfMacro))
   return Style.SpaceBeforeParensOptions.AfterIfMacros ||
  spaceRequiredBeforeParens(Right);
+if (Style.SpaceBeforeParens == FormatStyle::SBPO_Custom &&
+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;
+}
 if (Line.Type == LT_ObjCDecl)
   return true;
 if (Left.is(tok::semi))
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -936,6 +936,7 @@
Spacing.AfterFunctionDeclarationName);
 IO.mapOptional("AfterIfMacros", 

[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

omarahmed added a comment.

As I understand, the default behavior for when the user didn't use 
`SBPO_Custom` is to add a space in placement operators based on this issue 
. And, at the same time, the 
default behavior should be `APO_Never` when we have `SBPO_Custom` so that we 
handle other code that depends on that. the existing tests confirm this 
understanding. So, the current logic was added based on this understanding.




Comment at: clang/lib/Format/TokenAnnotator.cpp:3396
  spaceRequiredBeforeParens(Right);
+if (Style.SpaceBeforeParens == FormatStyle::SBPO_Custom &&
+Left.isOneOf(tok::kw_new, tok::kw_delete) &&

MyDeveloperDay wrote:
> shouldn't the very first part of this be? 
> 
> 
> ```
> if (Style.SpaceBeforeParensOptions.AfterPlacementOperator != 
> FormatStyle::SpaceBeforeParensCustom::APO_Leave)
> {
> 
> 
> 
> }
> ```
> 
> i.e. don't we want a zero change if someone says "Leave"
The current code logic, when we do not enter this suggested condition, will 
switch to [this 
condition](https://github.com/llvm/llvm-project/blob/c2bb2e5973acd24c45c1823e95e8e33003c59484/clang/lib/Format/TokenAnnotator.cpp#L3580).
 And will not leave the code as it is, but will change it.

I have thought of getting rid of this condition entirely, but it handles the 
default situation suggested here, not only for cpp but for other languages like 
js:
>>As I understand, the default behavior for when the user didn't use 
>>`SBPO_Custom` is to add a space in placement operators based on [this 
>>issue](https://github.com/llvm/llvm-project/issues/54703). And, at the same 
>>time, the default behavior should be `APO_Never` when we have `SBPO_Custom` 
>>so that we handle other code that depends on that. the existing tests confirm 
>>this understanding. So, the current logic was added based on this 
>>understanding.

I tried to add an extra condition to [this 
condition](https://github.com/llvm/llvm-project/blob/c2bb2e5973acd24c45c1823e95e8e33003c59484/clang/lib/Format/TokenAnnotator.cpp#L3580)
 to support the main logic that we are pursuing. so that it would be like that:
```
if (Style.SpaceBeforeParensOptions.AfterPlacementOperator != 
FormatStyle::SpaceBeforeParensCustom::APO_Leave && Left.isOneOf(tok::kw_new, 
tok::kw_delete))
```
But that wouldn't fix the leave situation, as it will be also at the end, reach 
[this 
part](https://github.com/llvm/llvm-project/blob/c2bb2e5973acd24c45c1823e95e8e33003c59484/clang/lib/Format/TokenAnnotator.cpp#L3598)
 which will  
```
return false
``` 
and force no space.

I think all of that was raised because all current 
"SpaceBeforeParanthesesOptions" are either true or false and this is the only 
enum

However, I completely agree with this logic which is to begin our checking for 
leave. But I think we should refactor all the "spaceBeforeParenthesesOptions" 
conditions to handle this and do that after all the options have been 
transformed to enums.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127270

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

omarahmed updated this revision to Diff 437966.
omarahmed marked an inline comment as not done.
omarahmed added a comment.

Refactor the tests and add new parsing logic for nested enums in 
dump_format_style.py


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127270

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/docs/tools/dump_format_style.py
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -10130,6 +10130,42 @@
 "void delete(link p);\n",
 format("void new (link p);\n"
"void delete (link p);\n"));
+
+  FormatStyle AfterPlacementOperator = getLLVMStyle();
+  AfterPlacementOperator.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  EXPECT_EQ(
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator,
+  FormatStyle::SpaceBeforeParensCustom::APO_Never);
+  verifyFormat("struct A {\n"
+   "  void *a;\n"
+   "  A(void *p) : a(new(p) int) {\n"
+   "new(p) int;\n"
+   "int *b = new(p) int;\n"
+   "int *c = new(p) int(3);\n"
+   "int *d = delete(p) int(3);\n"
+   "  }\n"
+   "};",
+   AfterPlacementOperator);
+  verifyFormat("void operator new(void *foo) ATTRIB;", AfterPlacementOperator);
+
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator =
+  FormatStyle::SpaceBeforeParensCustom::APO_Always;
+  verifyFormat("struct A {\n"
+   "  void *a;\n"
+   "  A(void *p) : a(new (p) int) {\n"
+   "new (p) int;\n"
+   "int *b = new (p) int;\n"
+   "int *c = new (p) int(3);\n"
+   "int *d = delete (p) int(3);\n"
+   "  }\n"
+   "};",
+   AfterPlacementOperator);
+  verifyFormat("void operator new(void *foo) ATTRIB;", AfterPlacementOperator);
+
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator =
+  FormatStyle::SpaceBeforeParensCustom::APO_Leave;
+  EXPECT_EQ("new (buf) int;", format("new (buf) int;", AfterPlacementOperator));
+  EXPECT_EQ("new(buf) int;", format("new(buf) int;", AfterPlacementOperator));
 }
 
 TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3393,6 +3393,18 @@
 if (Left.is(TT_IfMacro))
   return Style.SpaceBeforeParensOptions.AfterIfMacros ||
  spaceRequiredBeforeParens(Right);
+if (Style.SpaceBeforeParens == FormatStyle::SBPO_Custom &&
+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;
+}
 if (Line.Type == LT_ObjCDecl)
   return true;
 if (Left.is(tok::semi))
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -936,6 +936,7 @@
Spacing.AfterFunctionDeclarationName);
 IO.mapOptional("AfterIfMacros", Spacing.AfterIfMacros);
 IO.mapOptional("AfterOverloadedOperator", Spacing.AfterOverloadedOperator);
+IO.mapOptional("AfterPlacementOperator", Spacing.AfterPlacementOperator);
 IO.mapOptional("AfterRequiresInClause", Spacing.AfterRequiresInClause);
 IO.mapOptional("AfterRequiresInExpression",
Spacing.AfterRequiresInExpression);
@@ -944,6 +945,22 @@
   }
 };
 
+template <>
+struct MappingTraits<
+FormatStyle::SpaceBeforeParensCustom::AfterPlacementOperatorStyle> {
+  static void
+  mapping(IO ,
+  FormatStyle::SpaceBeforeParensCustom::AfterPlacementOperatorStyle
+  ) {
+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 {
   static void mapping(IO , 

[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

omarahmed added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:4261
 
+  **AfterPlacementOperator** (``AfterPlacementOperatorStyle``) 
:versionbadge:`clang-format 14`
+Defines in which cases to put a space between new/delete operators and 
opening parentheses.

MyDeveloperDay wrote:
> Are you making this by hand or by running tools/docs/dump_format_style.py?
I was doing it manually, as I didn't know there is a script that does that. I 
tried using the script but seems like it has a parsing error with this logic as 
it is the first time to use `enum` inside a `struct` in `format.h` file. So it 
seems that the parsing logic for this case needs to be added. I will try adding 
this parsing case to `dump_format_style.py` and add it to my next update for 
this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127270

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

omarahmed added a comment.

In D127270#3584200 , @curdeius wrote:

> Does this patch really fix https://github.com/llvm/llvm-project/issues/54703?
> If so, please add test for it. Otherwise remove the link from the summary 
> (and if possible handle it in another review).

It should cover this case too, I have added a test to verify that it covers it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127270

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

omarahmed updated this revision to Diff 437468.
omarahmed edited the summary of this revision.
omarahmed added a comment.

Add coverage for placement delete expressions and transform bool option to enum


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127270

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -10130,6 +10130,37 @@
 "void delete(link p);\n",
 format("void new (link p);\n"
"void delete (link p);\n"));
+
+  FormatStyle AfterPlacementOperator = getLLVMStyle();
+  AfterPlacementOperator.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  EXPECT_EQ(
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator,
+  FormatStyle::SpaceBeforeParensCustom::APO_Always);
+  verifyFormat("new (buf) T;", AfterPlacementOperator);
+  verifyFormat("T *p = new (buf) T;", AfterPlacementOperator);
+  verifyFormat("T *p = new (buf) T(3);", AfterPlacementOperator);
+  verifyFormat("T *p = delete (buf)T(3);", AfterPlacementOperator);
+  verifyFormat("struct A {\n"
+   "  void *a;\n"
+   "  A(void *p) : a(new (p) int) {}\n"
+   "};",
+   AfterPlacementOperator);
+
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator =
+  FormatStyle::SpaceBeforeParensCustom::APO_Never;
+  verifyFormat("new(buf) T;", AfterPlacementOperator);
+  verifyFormat("T *p = new(buf) T;", AfterPlacementOperator);
+  verifyFormat("T *p = delete(buf)T(3);", AfterPlacementOperator);
+  verifyFormat("struct A {\n"
+   "  void *a;\n"
+   "  A(void *p) : a(new(p) int) {}\n"
+   "};",
+   AfterPlacementOperator);
+
+  AfterPlacementOperator.SpaceBeforeParensOptions.AfterPlacementOperator =
+  FormatStyle::SpaceBeforeParensCustom::APO_Leave;
+  EXPECT_EQ("new (buf) T;", format("new (buf) T;", AfterPlacementOperator));
+  EXPECT_EQ("new(buf) T;", format("new(buf) T;", AfterPlacementOperator));
 }
 
 TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3393,6 +3393,18 @@
 if (Left.is(TT_IfMacro))
   return Style.SpaceBeforeParensOptions.AfterIfMacros ||
  spaceRequiredBeforeParens(Right);
+if (Style.SpaceBeforeParens == FormatStyle::SBPO_Custom &&
+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;
+}
 if (Line.Type == LT_ObjCDecl)
   return true;
 if (Left.is(tok::semi))
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -936,6 +936,7 @@
Spacing.AfterFunctionDeclarationName);
 IO.mapOptional("AfterIfMacros", Spacing.AfterIfMacros);
 IO.mapOptional("AfterOverloadedOperator", Spacing.AfterOverloadedOperator);
+IO.mapOptional("AfterPlacementOperator", Spacing.AfterPlacementOperator);
 IO.mapOptional("AfterRequiresInClause", Spacing.AfterRequiresInClause);
 IO.mapOptional("AfterRequiresInExpression",
Spacing.AfterRequiresInExpression);
@@ -944,6 +945,22 @@
   }
 };
 
+template <>
+struct MappingTraits<
+FormatStyle::SpaceBeforeParensCustom::AfterPlacementOperatorStyle> {
+  static void
+  mapping(IO ,
+  FormatStyle::SpaceBeforeParensCustom::AfterPlacementOperatorStyle
+  ) {
+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 {
   static void mapping(IO , FormatStyle::RawStringFormat ) {
 IO.mapOptional("Language", Format.Language);
Index: clang/include/clang/Format/Format.h
===
--- 

[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

omarahmed added inline comments.



Comment at: clang/lib/Format/Format.cpp:1314
   LLVMStyle.SpaceBeforeParensOptions.AfterIfMacros = true;
+  LLVMStyle.SpaceBeforeParensOptions.AfterPlacementNew = true;
   LLVMStyle.SpaceBeforeRangeBasedForLoopColon = true;

HazardyKnusperkeks wrote:
> This isn't needed, because the default CTor initializes it with true.
> Or you change that, I don't know right now why all other attributes are 
> initialized with false.
I think they are initialized with false so that when we come to [this 
case](https://github.com/llvm/llvm-project/blob/54ae4ca7550a81fd1fa9e484904d553af8fbb2fd/clang/lib/Format/Format.cpp#L1155),
 it breaks while all of them are false. (I am not sure so) I will try to add a 
test to cover 'SBPO_Never' but after converting it to enum.



Comment at: clang/lib/Format/TokenAnnotator.cpp:3399
+  return Style.SpaceBeforeParensOptions.AfterPlacementNew ||
+ spaceRequiredBeforeParens(Right);
 if (Line.Type == LT_ObjCDecl)

MyDeveloperDay wrote:
> what case is covered here by spaceRequiredBeforeParens(Right)?
I intended to cover this case: `new (buf) T` so basically placement new 
expressions, excluding the overloaded new operator function 
declarations/definitions like `T *operator new(size_t size) {}` and excluding 
function declarations/definitions named new like `T *new();`



Comment at: clang/unittests/Format/FormatTest.cpp:15276
 
+  FormatStyle SpacePlacementNew = getLLVMStyle();
+  SpacePlacementNew.SpaceBeforeParens = FormatStyle::SBPO_Custom;

MyDeveloperDay wrote:
> curdeius wrote:
> > Are there any tests with `AfterPlacementNew = false;`? Could you add those 
> > please?
> you should probably give yourself your own test, for SpacePlacementNew
I was thinking of covering `placement delete` too and adding the tests for this 
patch to [this 
test](https://github.com/llvm/llvm-project/blob/54ae4ca7550a81fd1fa9e484904d553af8fbb2fd/clang/unittests/Format/FormatTest.cpp#L10153)
 as it should add more cases to new/delete operators. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127270

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127270: [clang-format] Add space in placement new expression

[omar ahmed via Phabricator via cfe-commits]

omarahmed created this revision.
omarahmed added a reviewer: MyDeveloperDay.
Herald added a project: All.
omarahmed requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Add AfterPlacementNew option to SpaceBeforeParensOptions to have more control 
on placement new expressions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127270

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -15273,6 +15273,18 @@
   verifyFormat("M(std::size_t R, std::size_t C) : C(C), data(R) {}",
SpaceFuncDef);
 
+  FormatStyle SpacePlacementNew = getLLVMStyle();
+  SpacePlacementNew.SpaceBeforeParens = FormatStyle::SBPO_Custom;
+  SpacePlacementNew.SpaceBeforeParensOptions.AfterPlacementNew = true;
+  verifyFormat("new (buf) T;", SpacePlacementNew);
+  verifyFormat("T *p = new (buf) T;", SpacePlacementNew);
+  verifyFormat("T *p = new (buf) T(3);", SpacePlacementNew);
+  verifyFormat("T *new() {}", SpacePlacementNew);
+  verifyFormat("T *new();", SpacePlacementNew);
+  verifyFormat("T *operator new(size_t size) {}", SpacePlacementNew);
+  verifyFormat("new T;", SpacePlacementNew);
+  verifyFormat("T *p = new T;", SpacePlacementNew);
+
   FormatStyle SpaceIfMacros = getLLVMStyle();
   SpaceIfMacros.IfMacros.clear();
   SpaceIfMacros.IfMacros.push_back("MYIF");
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3393,6 +3393,10 @@
 if (Left.is(TT_IfMacro))
   return Style.SpaceBeforeParensOptions.AfterIfMacros ||
  spaceRequiredBeforeParens(Right);
+if (Left.is(tok::kw_new) && Right.isNot(TT_OverloadedOperatorLParen) &&
+!Line.MightBeFunctionDecl)
+  return Style.SpaceBeforeParensOptions.AfterPlacementNew ||
+ spaceRequiredBeforeParens(Right);
 if (Line.Type == LT_ObjCDecl)
   return true;
 if (Left.is(tok::semi))
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -935,6 +935,7 @@
 IO.mapOptional("AfterFunctionDeclarationName",
Spacing.AfterFunctionDeclarationName);
 IO.mapOptional("AfterIfMacros", Spacing.AfterIfMacros);
+IO.mapOptional("AfterPlacementNew", Spacing.AfterPlacementNew);
 IO.mapOptional("AfterOverloadedOperator", Spacing.AfterOverloadedOperator);
 IO.mapOptional("AfterRequiresInClause", Spacing.AfterRequiresInClause);
 IO.mapOptional("AfterRequiresInExpression",
@@ -1310,6 +1311,7 @@
   LLVMStyle.SpaceBeforeParensOptions.AfterControlStatements = true;
   LLVMStyle.SpaceBeforeParensOptions.AfterForeachMacros = true;
   LLVMStyle.SpaceBeforeParensOptions.AfterIfMacros = true;
+  LLVMStyle.SpaceBeforeParensOptions.AfterPlacementNew = true;
   LLVMStyle.SpaceBeforeRangeBasedForLoopColon = true;
   LLVMStyle.SpaceBeforeAssignmentOperators = true;
   LLVMStyle.SpaceBeforeCpp11BracedList = false;
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -3490,6 +3490,12 @@
 ///   
 /// \endcode
 bool AfterIfMacros;
+/// If ``true``, put space between placement new and opening parentheses.
+/// \code
+///true:  false:
+///new (buf) T;vs.new(buf) T;
+/// \endcode
+bool AfterPlacementNew;
 /// If ``true``, put a space between operator overloading and opening
 /// parentheses.
 /// \code
@@ -3530,8 +3536,9 @@
 : AfterControlStatements(false), AfterForeachMacros(false),
   AfterFunctionDeclarationName(false),
   AfterFunctionDefinitionName(false), AfterIfMacros(false),
-  AfterOverloadedOperator(false), AfterRequiresInClause(false),
-  AfterRequiresInExpression(false), BeforeNonEmptyParentheses(false) {}
+  AfterPlacementNew(true), AfterOverloadedOperator(false),
+  AfterRequiresInClause(false), AfterRequiresInExpression(false),
+  BeforeNonEmptyParentheses(false) {}
 
 bool operator==(const SpaceBeforeParensCustom ) const {
   return AfterControlStatements == Other.AfterControlStatements &&
@@ -3540,6 +3547,7 @@
  Other.AfterFunctionDeclarationName &&
  AfterFunctionDefinitionName == Other.AfterFunctionDefinitionName &&
  AfterIfMacros == Other.AfterIfMacros &&
+