[PATCH] D88227: [clang-format] Add a SpaceBeforePointerQualifiers style option

[Alexander Richardson via Phabricator via cfe-commits]

arichardson updated this revision to Diff 296689.
arichardson added a comment.

Use an enum config option instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88227

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
@@ -12236,6 +12236,80 @@
NoSpaceStyle);
 }
 
+TEST_F(FormatTest, ConfigurableSpaceAroundPointerQualifiers) {
+  FormatStyle Style = getLLVMStyle();
+
+  Style.PointerAlignment = FormatStyle::PAS_Left;
+  Style.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_Default;
+  verifyFormat("void* const* x = NULL;", Style);
+
+#define verifyQualifierSpaces(Code, Pointers, Qualifiers)  \
+  do { \
+Style.PointerAlignment = FormatStyle::Pointers;\
+Style.SpaceAroundPointerQualifiers = FormatStyle::Qualifiers;  \
+verifyFormat(Code, Style); \
+  } while (false)
+
+  verifyQualifierSpaces("void* const* x = NULL;", PAS_Left, SAPQ_Default);
+  verifyQualifierSpaces("void *const *x = NULL;", PAS_Right, SAPQ_Default);
+  verifyQualifierSpaces("void * const * x = NULL;", PAS_Middle, SAPQ_Default);
+
+  verifyQualifierSpaces("void* const* x = NULL;", PAS_Left, SAPQ_Before);
+  verifyQualifierSpaces("void * const *x = NULL;", PAS_Right, SAPQ_Before);
+  verifyQualifierSpaces("void * const * x = NULL;", PAS_Middle, SAPQ_Before);
+
+  verifyQualifierSpaces("void* const * x = NULL;", PAS_Left, SAPQ_After);
+  verifyQualifierSpaces("void *const *x = NULL;", PAS_Right, SAPQ_After);
+  verifyQualifierSpaces("void * const * x = NULL;", PAS_Middle, SAPQ_After);
+
+  verifyQualifierSpaces("void* const * x = NULL;", PAS_Left, SAPQ_Both);
+  verifyQualifierSpaces("void * const *x = NULL;", PAS_Right, SAPQ_Both);
+  verifyQualifierSpaces("void * const * x = NULL;", PAS_Middle, SAPQ_Both);
+
+#undef verifyQualifierSpaces
+
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.AttributeMacros.push_back("qualified");
+  Spaces.PointerAlignment = FormatStyle::PAS_Right;
+  Spaces.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_Default;
+  verifyFormat("SomeType *volatile *a = NULL;", Spaces);
+  verifyFormat("SomeType *__attribute__((attr)) *a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  Spaces.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_Before;
+  verifyFormat("SomeType * volatile *a = NULL;", Spaces);
+  verifyFormat("SomeType * __attribute__((attr)) *a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+
+  // Check that SAPQ_Before doesn't result in extra spaces for PAS_Left.
+  Spaces.PointerAlignment = FormatStyle::PAS_Left;
+  Spaces.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_Before;
+  verifyFormat("SomeType* volatile* a = NULL;", Spaces);
+  verifyFormat("SomeType* __attribute__((attr))* a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  // However, setting it to SAPQ_After should add spaces after __attribute, etc.
+  Spaces.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_After;
+  verifyFormat("SomeType* volatile * a = NULL;", Spaces);
+  verifyFormat("SomeType* __attribute__((attr)) * a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+
+  // PAS_Middle should not have any noticeable changes even for SAPQ_Both
+  Spaces.PointerAlignment = FormatStyle::PAS_Middle;
+  Spaces.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_After;
+  verifyFormat("SomeType * volatile * a = NULL;", Spaces);
+  verifyFormat("SomeType * __attribute__((attr)) * a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+}
+
 TEST_F(FormatTest, AlignConsecutiveMacros) {
   FormatStyle Style = getLLVMStyle();
   Style.AlignConsecutiveAssignments = true;
@@ -14324,6 +14398,16 @@
   CHECK_PARSE("AllowShortFunctionsOnASingleLine: true",
   AllowShortFunctionsOnASingleLine, FormatStyle::SFS_All);
 
+  Style.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_Default;
+  CHECK_PARSE("SpaceAroundPointerQualifiers: Never",
+  SpaceAroundPointerQualifiers, FormatStyle::SAPQ_Default);
+  

[PATCH] D88227: [clang-format] Add a SpaceBeforePointerQualifiers style option

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw added a comment.

Perhaps PointerAlignment should be an enum of Left, Middle, Right?
We maintain an internal patch that adds "ReferenceAlignment" as separate from 
PointerAlignment. So one could have:

  PointerAlignment: Middle
  ReferenceAlignment: Left


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88227

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

[PATCH] D88227: [clang-format] Add a SpaceBeforePointerQualifiers style option

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added inline comments.



Comment at: clang/include/clang/Format/Format.h:2181
+  /// \endcode
+  bool SpaceBeforePointerQualifiers;
+

MyDeveloperDay wrote:
> I wonder if a request would ever say they wanted this Space before to be 
> Left/Middle/Right? perhaps this should not be a bool?
> 
> ```
> void* const *x;
> void * const *x;
> void *const *x;
> ```
> 
> ```
> void* const *x;
> void * const *x;
> void *const *x;
> ```
> 
> ```
> void* const * x;
> void * const * x;
> void *const * x;
> ```
> 
> ```
> void* const* x;
> void * const* x;
> void *const* x;
> ```
Shouldn't be too hard to implement so why not. How about 
PointerAlignmentForQualifiers as the setting name?

We will have to ensure that the default value matches PointerAlignment if it's 
not set since otherwise you'd get really weird code formatting after updating 
clang-format if you don't change the config file.
Are there and other settings that behave this way? If not I think I'd rather 
keep the boolean option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88227

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

[PATCH] D88227: [clang-format] Add a SpaceBeforePointerQualifiers style option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/include/clang/Format/Format.h:2181
+  /// \endcode
+  bool SpaceBeforePointerQualifiers;
+

I wonder if a request would ever say they wanted this Space before to be 
Left/Middle/Right? perhaps this should not be a bool?

```
void* const *x;
void * const *x;
void *const *x;
```

```
void* const *x;
void * const *x;
void *const *x;
```

```
void* const * x;
void * const * x;
void *const * x;
```

```
void* const* x;
void * const* x;
void *const* x;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88227

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

[PATCH] D88227: [clang-format] Add a SpaceBeforePointerQualifiers style option

[Alexander Richardson via Phabricator via cfe-commits]

arichardson updated this revision to Diff 294090.
arichardson added a comment.

- Add a multi-variable-decl test (depends on D88239 
)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88227

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
@@ -12104,6 +12104,47 @@
NoSpaceStyle);
 }
 
+TEST_F(FormatTest, ConfigurableSpaceBeforePointerQualifiers) {
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.AttributeMacros.push_back("qualified");
+  verifyFormat("SomeType *const *a = NULL;", Spaces);
+  verifyFormat("SomeType *volatile *a = NULL;", Spaces);
+  verifyFormat("SomeType *__attribute__((attr)) *a = NULL;", Spaces);
+  verifyFormat("SomeType *qualified *a = NULL, *const *b;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+
+  Spaces.SpaceBeforePointerQualifiers = true;
+  verifyFormat("SomeType * const *a = NULL;", Spaces);
+  verifyFormat("SomeType * volatile *a = NULL;", Spaces);
+  verifyFormat("SomeType * __attribute__((attr)) *a = NULL;", Spaces);
+  verifyFormat("SomeType * qualified *a = NULL, * const *b;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+
+  // Check that setting SpaceBeforePointerQualifiers doesn't result in extra
+  // spaces for PAS_Left/PAS_Middle.
+  Spaces.PointerAlignment = FormatStyle::PAS_Left;
+  verifyFormat("SomeType* const* a = NULL;", Spaces);
+  verifyFormat("SomeType* volatile* a = NULL;", Spaces);
+  verifyFormat("SomeType* __attribute__((attr))* a = NULL;", Spaces);
+  verifyFormat("SomeType* qualified* a = NULL, * const* b;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+
+  Spaces.PointerAlignment = FormatStyle::PAS_Middle;
+  verifyFormat("SomeType * const * a = NULL;", Spaces);
+  verifyFormat("SomeType * volatile * a = NULL;", Spaces);
+  verifyFormat("SomeType * __attribute__((attr)) * a = NULL;", Spaces);
+  verifyFormat("SomeType * qualified * a = NULL, * const * b;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+}
+
 TEST_F(FormatTest, AlignConsecutiveMacros) {
   FormatStyle Style = getLLVMStyle();
   Style.AlignConsecutiveAssignments = true;
@@ -14003,6 +14044,7 @@
   CHECK_PARSE_BOOL(SpaceBeforeCpp11BracedList);
   CHECK_PARSE_BOOL(SpaceBeforeCtorInitializerColon);
   CHECK_PARSE_BOOL(SpaceBeforeInheritanceColon);
+  CHECK_PARSE_BOOL(SpaceBeforePointerQualifiers);
   CHECK_PARSE_BOOL(SpaceBeforeRangeBasedForLoopColon);
   CHECK_PARSE_BOOL(SpaceBeforeSquareBrackets);
   CHECK_PARSE_BOOL(UseCRLF);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2873,6 +2873,8 @@
 return true;
   if (Left.is(TT_PointerOrReference))
 return Right.Tok.isLiteral() || Right.is(TT_BlockComment) ||
+   (Style.SpaceBeforePointerQualifiers &&
+Right.canBePointerOrReferenceQualifier()) ||
(Right.isOneOf(Keywords.kw_override, Keywords.kw_final) &&
 !Right.is(TT_StartOfName)) ||
(Right.is(tok::l_brace) && Right.is(BK_Block)) ||
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -598,6 +598,8 @@
 IO.mapOptional("SpaceBeforeInheritanceColon",
Style.SpaceBeforeInheritanceColon);
 IO.mapOptional("SpaceBeforeParens", Style.SpaceBeforeParens);
+IO.mapOptional("SpaceBeforePointerQualifiers",
+   Style.SpaceBeforePointerQualifiers);
 IO.mapOptional("SpaceBeforeRangeBasedForLoopColon",
Style.SpaceBeforeRangeBasedForLoopColon);
 IO.mapOptional("SpaceInEmptyBlock", Style.SpaceInEmptyBlock);
@@ -938,6 +940,7 @@
   LLVMStyle.SpaceBeforeCtorInitializerColon = true;
   LLVMStyle.SpaceBeforeInheritanceColon = true;
   LLVMStyle.SpaceBeforeParens = FormatStyle::SBPO_ControlStatements;
+  LLVMStyle.SpaceBeforePointerQualifiers = false;
   LLVMStyle.SpaceBeforeRangeBasedForLoopColon = true;
   LLVMStyle.SpaceBeforeAssignmentOperators = true;
   LLVMStyle.SpaceBeforeCpp11BracedList = false;
Index: clang/include/clang/Format/Format.h

[PATCH] D88227: [clang-format] Add a SpaceBeforePointerQualifiers style option

[Alexander Richardson via Phabricator via cfe-commits]

arichardson updated this revision to Diff 294049.
arichardson marked 3 inline comments as done.
arichardson added a comment.

merge if with existing condition


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88227

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
@@ -12104,6 +12104,47 @@
NoSpaceStyle);
 }
 
+TEST_F(FormatTest, ConfigurableSpaceBeforePointerQualifiers) {
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.AttributeMacros.push_back("qualified");
+  verifyFormat("SomeType *const *a = NULL;", Spaces);
+  verifyFormat("SomeType *volatile *a = NULL;", Spaces);
+  verifyFormat("SomeType *__attribute__((attr)) *a = NULL;", Spaces);
+  verifyFormat("SomeType *qualified *a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+
+  Spaces.SpaceBeforePointerQualifiers = true;
+  verifyFormat("SomeType * const *a = NULL;", Spaces);
+  verifyFormat("SomeType * volatile *a = NULL;", Spaces);
+  verifyFormat("SomeType * __attribute__((attr)) *a = NULL;", Spaces);
+  verifyFormat("SomeType * qualified *a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+
+  // Check that setting SpaceBeforePointerQualifiers doesn't result in extra
+  // spaces for PAS_Left/PAS_Middle.
+  Spaces.PointerAlignment = FormatStyle::PAS_Left;
+  verifyFormat("SomeType* const* a = NULL;", Spaces);
+  verifyFormat("SomeType* volatile* a = NULL;", Spaces);
+  verifyFormat("SomeType* __attribute__((attr))* a = NULL;", Spaces);
+  verifyFormat("SomeType* qualified* a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+
+  Spaces.PointerAlignment = FormatStyle::PAS_Middle;
+  verifyFormat("SomeType * const * a = NULL;", Spaces);
+  verifyFormat("SomeType * volatile * a = NULL;", Spaces);
+  verifyFormat("SomeType * __attribute__((attr)) * a = NULL;", Spaces);
+  verifyFormat("SomeType * qualified * a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+}
+
 TEST_F(FormatTest, AlignConsecutiveMacros) {
   FormatStyle Style = getLLVMStyle();
   Style.AlignConsecutiveAssignments = true;
@@ -14003,6 +14044,7 @@
   CHECK_PARSE_BOOL(SpaceBeforeCpp11BracedList);
   CHECK_PARSE_BOOL(SpaceBeforeCtorInitializerColon);
   CHECK_PARSE_BOOL(SpaceBeforeInheritanceColon);
+  CHECK_PARSE_BOOL(SpaceBeforePointerQualifiers);
   CHECK_PARSE_BOOL(SpaceBeforeRangeBasedForLoopColon);
   CHECK_PARSE_BOOL(SpaceBeforeSquareBrackets);
   CHECK_PARSE_BOOL(UseCRLF);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2873,6 +2873,8 @@
 return true;
   if (Left.is(TT_PointerOrReference))
 return Right.Tok.isLiteral() || Right.is(TT_BlockComment) ||
+   (Style.SpaceBeforePointerQualifiers &&
+Right.canBePointerOrReferenceQualifier()) ||
(Right.isOneOf(Keywords.kw_override, Keywords.kw_final) &&
 !Right.is(TT_StartOfName)) ||
(Right.is(tok::l_brace) && Right.is(BK_Block)) ||
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -598,6 +598,8 @@
 IO.mapOptional("SpaceBeforeInheritanceColon",
Style.SpaceBeforeInheritanceColon);
 IO.mapOptional("SpaceBeforeParens", Style.SpaceBeforeParens);
+IO.mapOptional("SpaceBeforePointerQualifiers",
+   Style.SpaceBeforePointerQualifiers);
 IO.mapOptional("SpaceBeforeRangeBasedForLoopColon",
Style.SpaceBeforeRangeBasedForLoopColon);
 IO.mapOptional("SpaceInEmptyBlock", Style.SpaceInEmptyBlock);
@@ -938,6 +940,7 @@
   LLVMStyle.SpaceBeforeCtorInitializerColon = true;
   LLVMStyle.SpaceBeforeInheritanceColon = true;
   LLVMStyle.SpaceBeforeParens = FormatStyle::SBPO_ControlStatements;
+  LLVMStyle.SpaceBeforePointerQualifiers = false;
   LLVMStyle.SpaceBeforeRangeBasedForLoopColon = true;
   LLVMStyle.SpaceBeforeAssignmentOperators = true;
   LLVMStyle.SpaceBeforeCpp11BracedList = false;
Index: clang/include/clang/Format/Format.h
===
--- 

[PATCH] D88227: [clang-format] Add a SpaceBeforePointerQualifiers style option

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2874-2889
+  if (Left.is(TT_PointerOrReference)) {
+if (Style.SpaceBeforePointerQualifiers &&
+Right.canBePointerOrReferenceQualifier())
+  return true;
 return Right.Tok.isLiteral() || Right.is(TT_BlockComment) ||
(Right.isOneOf(Keywords.kw_override, Keywords.kw_final) &&
 !Right.is(TT_StartOfName)) ||

arichardson wrote:
> jrtc27 wrote:
> > arichardson wrote:
> > > jrtc27 wrote:
> > > > 
> > > I feel like moving it here could in theory miss some cases. Also the 
> > > condition is already barely comprehensible (I didn't attempt to 
> > > understand which special cases all these conditions are for) and I don't 
> > > feel like making it more complex.
> > > 
> > > If clang-format has identified that this */& token is a 
> > > pointer/reference, and the next token is something that can be paresed as 
> > > a pointer qualifier, shouldn't we trust the parser and simply look at the 
> > > format option? It also IMO makes the code slightly easier to understand.
> > It's a series of `A || B || C || ...`, and you just added an `if (X) return 
> > true;` above, so changing it to `A || B || C || X || ...` is entirely 
> > equivalent, no need to understand what any of the other expressions in the 
> > giant OR are.
> Never mind, I thought your suggestion it was inside nested parens. It's 
> equivalent just avoids the braces.
Uh of course with `||` added on the end of that new sub-expression, managed to 
omit that...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88227

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

[PATCH] D88227: [clang-format] Add a SpaceBeforePointerQualifiers style option

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2874-2889
+  if (Left.is(TT_PointerOrReference)) {
+if (Style.SpaceBeforePointerQualifiers &&
+Right.canBePointerOrReferenceQualifier())
+  return true;
 return Right.Tok.isLiteral() || Right.is(TT_BlockComment) ||
(Right.isOneOf(Keywords.kw_override, Keywords.kw_final) &&
 !Right.is(TT_StartOfName)) ||

arichardson wrote:
> jrtc27 wrote:
> > 
> I feel like moving it here could in theory miss some cases. Also the 
> condition is already barely comprehensible (I didn't attempt to understand 
> which special cases all these conditions are for) and I don't feel like 
> making it more complex.
> 
> If clang-format has identified that this */& token is a pointer/reference, 
> and the next token is something that can be paresed as a pointer qualifier, 
> shouldn't we trust the parser and simply look at the format option? It also 
> IMO makes the code slightly easier to understand.
Never mind, I thought your suggestion it was inside nested parens. It's 
equivalent just avoids the braces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88227

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

[PATCH] D88227: [clang-format] Add a SpaceBeforePointerQualifiers style option

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2874-2889
+  if (Left.is(TT_PointerOrReference)) {
+if (Style.SpaceBeforePointerQualifiers &&
+Right.canBePointerOrReferenceQualifier())
+  return true;
 return Right.Tok.isLiteral() || Right.is(TT_BlockComment) ||
(Right.isOneOf(Keywords.kw_override, Keywords.kw_final) &&
 !Right.is(TT_StartOfName)) ||

arichardson wrote:
> jrtc27 wrote:
> > 
> I feel like moving it here could in theory miss some cases. Also the 
> condition is already barely comprehensible (I didn't attempt to understand 
> which special cases all these conditions are for) and I don't feel like 
> making it more complex.
> 
> If clang-format has identified that this */& token is a pointer/reference, 
> and the next token is something that can be paresed as a pointer qualifier, 
> shouldn't we trust the parser and simply look at the format option? It also 
> IMO makes the code slightly easier to understand.
It's a series of `A || B || C || ...`, and you just added an `if (X) return 
true;` above, so changing it to `A || B || C || X || ...` is entirely 
equivalent, no need to understand what any of the other expressions in the 
giant OR are.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88227

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

[PATCH] D88227: [clang-format] Add a SpaceBeforePointerQualifiers style option

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2874-2889
+  if (Left.is(TT_PointerOrReference)) {
+if (Style.SpaceBeforePointerQualifiers &&
+Right.canBePointerOrReferenceQualifier())
+  return true;
 return Right.Tok.isLiteral() || Right.is(TT_BlockComment) ||
(Right.isOneOf(Keywords.kw_override, Keywords.kw_final) &&
 !Right.is(TT_StartOfName)) ||

jrtc27 wrote:
> 
I feel like moving it here could in theory miss some cases. Also the condition 
is already barely comprehensible (I didn't attempt to understand which special 
cases all these conditions are for) and I don't feel like making it more 
complex.

If clang-format has identified that this */& token is a pointer/reference, and 
the next token is something that can be paresed as a pointer qualifier, 
shouldn't we trust the parser and simply look at the format option? It also IMO 
makes the code slightly easier to understand.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88227

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

[PATCH] D88227: [clang-format] Add a SpaceBeforePointerQualifiers style option

[Jessica Clarke via Phabricator via cfe-commits]

jrtc27 added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:2874-2889
+  if (Left.is(TT_PointerOrReference)) {
+if (Style.SpaceBeforePointerQualifiers &&
+Right.canBePointerOrReferenceQualifier())
+  return true;
 return Right.Tok.isLiteral() || Right.is(TT_BlockComment) ||
(Right.isOneOf(Keywords.kw_override, Keywords.kw_final) &&
 !Right.is(TT_StartOfName)) ||




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88227

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

[PATCH] D88227: [clang-format] Add a SpaceBeforePointerQualifiers style option

[Alexander Richardson via Phabricator via cfe-commits]

arichardson created this revision.
arichardson added reviewers: JakeMerdichAMD, MyDeveloperDay, jrtc27.
Herald added subscribers: cfe-commits, krytarowski, emaste.
Herald added a project: clang.
arichardson requested review of this revision.

Some projects (e.g. FreeBSD) align pointers to the right but expect a
space between the '*' and any pointer qualifiers such as const.

SpaceBeforePointerQualifiers = false (LLVM style):   void *const x = NULL;
SpaceBeforePointerQualifiers = true (FreeBSD style): void * const x = NULL;


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88227

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
@@ -12104,6 +12104,47 @@
NoSpaceStyle);
 }
 
+TEST_F(FormatTest, ConfigurableSpaceBeforePointerQualifiers) {
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.AttributeMacros.push_back("qualified");
+  verifyFormat("SomeType *const *a = NULL;", Spaces);
+  verifyFormat("SomeType *volatile *a = NULL;", Spaces);
+  verifyFormat("SomeType *__attribute__((attr)) *a = NULL;", Spaces);
+  verifyFormat("SomeType *qualified *a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+
+  Spaces.SpaceBeforePointerQualifiers = true;
+  verifyFormat("SomeType * const *a = NULL;", Spaces);
+  verifyFormat("SomeType * volatile *a = NULL;", Spaces);
+  verifyFormat("SomeType * __attribute__((attr)) *a = NULL;", Spaces);
+  verifyFormat("SomeType * qualified *a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+
+  // Check that setting SpaceBeforePointerQualifiers doesn't result in extra
+  // spaces for PAS_Left/PAS_Middle.
+  Spaces.PointerAlignment = FormatStyle::PAS_Left;
+  verifyFormat("SomeType* const* a = NULL;", Spaces);
+  verifyFormat("SomeType* volatile* a = NULL;", Spaces);
+  verifyFormat("SomeType* __attribute__((attr))* a = NULL;", Spaces);
+  verifyFormat("SomeType* qualified* a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+
+  Spaces.PointerAlignment = FormatStyle::PAS_Middle;
+  verifyFormat("SomeType * const * a = NULL;", Spaces);
+  verifyFormat("SomeType * volatile * a = NULL;", Spaces);
+  verifyFormat("SomeType * __attribute__((attr)) * a = NULL;", Spaces);
+  verifyFormat("SomeType * qualified * a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+}
+
 TEST_F(FormatTest, AlignConsecutiveMacros) {
   FormatStyle Style = getLLVMStyle();
   Style.AlignConsecutiveAssignments = true;
@@ -14003,6 +14044,7 @@
   CHECK_PARSE_BOOL(SpaceBeforeCpp11BracedList);
   CHECK_PARSE_BOOL(SpaceBeforeCtorInitializerColon);
   CHECK_PARSE_BOOL(SpaceBeforeInheritanceColon);
+  CHECK_PARSE_BOOL(SpaceBeforePointerQualifiers);
   CHECK_PARSE_BOOL(SpaceBeforeRangeBasedForLoopColon);
   CHECK_PARSE_BOOL(SpaceBeforeSquareBrackets);
   CHECK_PARSE_BOOL(UseCRLF);
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2871,7 +2871,10 @@
(Style.PointerAlignment != FormatStyle::PAS_Right &&
 !Line.IsMultiVariableDeclStmt)))
 return true;
-  if (Left.is(TT_PointerOrReference))
+  if (Left.is(TT_PointerOrReference)) {
+if (Style.SpaceBeforePointerQualifiers &&
+Right.canBePointerOrReferenceQualifier())
+  return true;
 return Right.Tok.isLiteral() || Right.is(TT_BlockComment) ||
(Right.isOneOf(Keywords.kw_override, Keywords.kw_final) &&
 !Right.is(TT_StartOfName)) ||
@@ -2883,6 +2886,7 @@
 Left.Previous &&
 !Left.Previous->isOneOf(tok::l_paren, tok::coloncolon,
 tok::l_square));
+  }
   // Ensure right pointer alignement with ellipsis e.g. int *...P
   if (Left.is(tok::ellipsis) && Left.Previous &&
   Left.Previous->isOneOf(tok::star, tok::amp, tok::ampamp))
Index: clang/lib/Format/Format.cpp
===
--- clang/lib/Format/Format.cpp
+++ clang/lib/Format/Format.cpp
@@ -598,6 +598,8 @@
 IO.mapOptional("SpaceBeforeInheritanceColon",
Style.SpaceBeforeInheritanceColon);
 IO.mapOptional("SpaceBeforeParens", Style.SpaceBeforeParens);
+IO.mapOptional("SpaceBeforePointerQualifiers",
+