[PATCH] D66384: [clang-format] Fix a bug that joins template closer and =

[Owen Pan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL369214: [clang-format] Fix a bug that joins template closer 
and = (authored by owenpan, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66384?vs=215763&id=215788#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D66384

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


Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -2870,7 +2870,7 @@
   Left.isOneOf(tok::arrow, tok::period, tok::arrowstar, tok::periodstar) ||
   (Right.is(tok::period) && Right.isNot(TT_DesignatedInitializerPeriod)))
 return false;
-  if (!Style.SpaceBeforeAssignmentOperators &&
+  if (!Style.SpaceBeforeAssignmentOperators && Left.isNot(TT_TemplateCloser) &&
   Right.getPrecedence() == prec::Assignment)
 return false;
   if (Style.Language == FormatStyle::LK_Java && Right.is(tok::coloncolon) &&
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -6620,8 +6620,15 @@
 
   verifyFormat("A> a;", getChromiumStyle(FormatStyle::LK_Cpp));
 
-  verifyFormat("int i = a<1> >> 1;");
+  // template closer followed by a token that starts with > or =
   verifyFormat("bool b = a<1> > 1;");
+  verifyFormat("bool b = a<1> >= 1;");
+  verifyFormat("int i = a<1> >> 1;");
+  FormatStyle Style = getLLVMStyle();
+  Style.SpaceBeforeAssignmentOperators = false;
+  verifyFormat("bool b= a<1> == 1;", Style);
+  verifyFormat("a = 1;", Style);
+  verifyFormat("a >>= 1;", Style);
 
   verifyFormat("test >> a >> b;");
   verifyFormat("test << a >> b;");
Index: cfe/trunk/docs/ClangFormatStyleOptions.rst
===
--- cfe/trunk/docs/ClangFormatStyleOptions.rst
+++ cfe/trunk/docs/ClangFormatStyleOptions.rst
@@ -2050,8 +2050,8 @@
   .. code-block:: c++
 
  true:  false:
- int a = 5; vs. int a=5;
- a += 42a+=42;
+ int a = 5; vs. int a= 5;
+ a += 42;   a+= 42;
 
 **SpaceBeforeCpp11BracedList** (``bool``)
   If ``true``, a space will be inserted before a C++11 braced list
Index: cfe/trunk/include/clang/Format/Format.h
===
--- cfe/trunk/include/clang/Format/Format.h
+++ cfe/trunk/include/clang/Format/Format.h
@@ -1738,8 +1738,8 @@
   /// If ``false``, spaces will be removed before assignment operators.
   /// \code
   ///true:  false:
-  ///int a = 5; vs. int a=5;
-  ///a += 42a+=42;
+  ///int a = 5; vs. int a= 5;
+  ///a += 42;   a+= 42;
   /// \endcode
   bool SpaceBeforeAssignmentOperators;
 


Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -2870,7 +2870,7 @@
   Left.isOneOf(tok::arrow, tok::period, tok::arrowstar, tok::periodstar) ||
   (Right.is(tok::period) && Right.isNot(TT_DesignatedInitializerPeriod)))
 return false;
-  if (!Style.SpaceBeforeAssignmentOperators &&
+  if (!Style.SpaceBeforeAssignmentOperators && Left.isNot(TT_TemplateCloser) &&
   Right.getPrecedence() == prec::Assignment)
 return false;
   if (Style.Language == FormatStyle::LK_Java && Right.is(tok::coloncolon) &&
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -6620,8 +6620,15 @@
 
   verifyFormat("A> a;", getChromiumStyle(FormatStyle::LK_Cpp));
 
-  verifyFormat("int i = a<1> >> 1;");
+  // template closer followed by a token that starts with > or =
   verifyFormat("bool b = a<1> > 1;");
+  verifyFormat("bool b = a<1> >= 1;");
+  verifyFormat("int i = a<1> >> 1;");
+  FormatStyle Style = getLLVMStyle();
+  Style.SpaceBeforeAssignmentOperators = false;
+  verifyFormat("bool b= a<1> == 1;", Style);
+  verifyFormat("a = 1;", Style);
+  verifyFormat("a >>= 1;", Style);
 
   verifyFormat("test >> a >> b;");
   verifyFormat("test << a >> b;");
Index: cfe/trunk/docs/ClangFormatStyleOptions.rst

[PATCH] D66384: [clang-format] Fix a bug that joins template closer and =

[Owen Pan via Phabricator via cfe-commits]

owenpan marked an inline comment as done.
owenpan added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:6631
+  verifyFormat("a = 1;", Style);
+  verifyFormat("a >>= 1;", Style);
 

Quuxplusone wrote:
> Actually, could you add a test case specifically for the troublesome 
> `enable_if_t` pattern? Just in case any future option treats 
> default-template-arguments any differently from assignment-statements, 
> whitespace-wise.
> 
> verifyformat("template = 0>");
> 
I think it duplicates `a = 1;` on Line 6630 because they both test the 
insertion of a space between a template closer `>` and an assignment operator 
`=` that follows.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66384



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

[PATCH] D66384: [clang-format] Fix a bug that joins template closer and =

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

In D66384#1634405 , @Quuxplusone wrote:

> although I would still question whether 
> `Style.SpaceBeforeAssignmentOperators` is providing anyone any benefit at all.


See here  
for the patch. I don't know why the option was added but think it's probably 
outdated and should be deprecated.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66384



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

[PATCH] D66384: [clang-format] Fix a bug that joins template closer and =

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

LGTM too, although I would still question whether 
`Style.SpaceBeforeAssignmentOperators` is providing anyone any benefit at all.




Comment at: clang/unittests/Format/FormatTest.cpp:6631
+  verifyFormat("a = 1;", Style);
+  verifyFormat("a >>= 1;", Style);
 

Actually, could you add a test case specifically for the troublesome 
`enable_if_t` pattern? Just in case any future option treats 
default-template-arguments any differently from assignment-statements, 
whitespace-wise.

verifyformat("template = 0>");



Repository:
  rC Clang

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

https://reviews.llvm.org/D66384



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

[PATCH] D66384: [clang-format] Fix a bug that joins template closer and =

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

LGTM too, although I would still question whether 
`Style.SpaceBeforeAssignmentOperators` is providing anyone any benefit at all.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66384



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

[PATCH] D66384: [clang-format] Fix a bug that joins template closer and =

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D66384



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

[PATCH] D66384: [clang-format] Fix a bug that joins template closer and =

[Owen Pan via Phabricator via cfe-commits]

owenpan created this revision.
owenpan added reviewers: Quuxplusone, MyDeveloperDay, sammccall, klimek, 
djasper.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Also fixes the documentation for `SpaceBeforeAssignmentOperators`.

See the discussion here .


Repository:
  rC Clang

https://reviews.llvm.org/D66384

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  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
@@ -6620,8 +6620,15 @@
 
   verifyFormat("A> a;", getChromiumStyle(FormatStyle::LK_Cpp));
 
-  verifyFormat("int i = a<1> >> 1;");
+  // template closer followed by a token that starts with > or =
   verifyFormat("bool b = a<1> > 1;");
+  verifyFormat("bool b = a<1> >= 1;");
+  verifyFormat("int i = a<1> >> 1;");
+  FormatStyle Style = getLLVMStyle();
+  Style.SpaceBeforeAssignmentOperators = false;
+  verifyFormat("bool b= a<1> == 1;", Style);
+  verifyFormat("a = 1;", Style);
+  verifyFormat("a >>= 1;", Style);
 
   verifyFormat("test >> a >> b;");
   verifyFormat("test << a >> b;");
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2870,7 +2870,7 @@
   Left.isOneOf(tok::arrow, tok::period, tok::arrowstar, tok::periodstar) ||
   (Right.is(tok::period) && Right.isNot(TT_DesignatedInitializerPeriod)))
 return false;
-  if (!Style.SpaceBeforeAssignmentOperators &&
+  if (!Style.SpaceBeforeAssignmentOperators && Left.isNot(TT_TemplateCloser) &&
   Right.getPrecedence() == prec::Assignment)
 return false;
   if (Style.Language == FormatStyle::LK_Java && Right.is(tok::coloncolon) &&
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1738,8 +1738,8 @@
   /// If ``false``, spaces will be removed before assignment operators.
   /// \code
   ///true:  false:
-  ///int a = 5; vs. int a=5;
-  ///a += 42a+=42;
+  ///int a = 5; vs. int a= 5;
+  ///a += 42;   a+= 42;
   /// \endcode
   bool SpaceBeforeAssignmentOperators;
 
Index: clang/docs/ClangFormatStyleOptions.rst
===
--- clang/docs/ClangFormatStyleOptions.rst
+++ clang/docs/ClangFormatStyleOptions.rst
@@ -2050,8 +2050,8 @@
   .. code-block:: c++
 
  true:  false:
- int a = 5; vs. int a=5;
- a += 42a+=42;
+ int a = 5; vs. int a= 5;
+ a += 42;   a+= 42;
 
 **SpaceBeforeCpp11BracedList** (``bool``)
   If ``true``, a space will be inserted before a C++11 braced list


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6620,8 +6620,15 @@
 
   verifyFormat("A> a;", getChromiumStyle(FormatStyle::LK_Cpp));
 
-  verifyFormat("int i = a<1> >> 1;");
+  // template closer followed by a token that starts with > or =
   verifyFormat("bool b = a<1> > 1;");
+  verifyFormat("bool b = a<1> >= 1;");
+  verifyFormat("int i = a<1> >> 1;");
+  FormatStyle Style = getLLVMStyle();
+  Style.SpaceBeforeAssignmentOperators = false;
+  verifyFormat("bool b= a<1> == 1;", Style);
+  verifyFormat("a = 1;", Style);
+  verifyFormat("a >>= 1;", Style);
 
   verifyFormat("test >> a >> b;");
   verifyFormat("test << a >> b;");
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -2870,7 +2870,7 @@
   Left.isOneOf(tok::arrow, tok::period, tok::arrowstar, tok::periodstar) ||
   (Right.is(tok::period) && Right.isNot(TT_DesignatedInitializerPeriod)))
 return false;
-  if (!Style.SpaceBeforeAssignmentOperators &&
+  if (!Style.SpaceBeforeAssignmentOperators && Left.isNot(TT_TemplateCloser) &&
   Right.getPrecedence() == prec::Assignment)
 return false;
   if (Style.Language == FormatStyle::LK_Java && Right.is(tok::coloncolon) &&
Index: clang/include/clang/Format/Format.h
===
--- clang/include/clang/Format/Format.h
+++ clang/include/clang/Format/Format.h
@@ -1738,8 +1738,8 @@
   /// If ``false``, spaces will be removed before assignment