[PATCH] D66332: [clang-format] Fix the bug that joins template closer and > or >>

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

The culprit is `AnnotatingParser::parseAngle()` in 
clang/lib/Format/TokenAnnotator.cpp. This commit merely uncovered it. :)

Upon reading a `<` token, `parseAngle()` tries to scan the rest of the line to 
find a matching `>`. If found, it's given the type `TT_TemplateCloser`. This 
should be fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66332



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

[PATCH] D66332: [clang-format] Fix the bug that joins template closer and > or >>

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

This seems to have caused https://bugs.llvm.org/show_bug.cgi?id=45218
Please take a look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66332



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

[PATCH] D66332: [clang-format] Fix the bug that joins template closer and > or >>

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

> Are there any other clang-format options that might lead to a lack-of-space 
> before `>`, `>=`, `==`, or `=`? I brought up `enable_if_t=0` 
> because that specifically is a construction I've run into in my own code, 
> even though I've never tried to clang-format it, let alone have it formatted 
> incorrectly.

Any token that starts with `>`, e.g., `>`, `>>`, `>=`, and `>>=`, are already 
taken care of by this patch. For tokens starting with `=`, only the assignment 
operator `=` has a problem and it only occurs when 
`SpaceBeforeAssignmentOperators` is set to true.

I can insert a space between a template closer `>` and an assignment operator 
`=` despite the value of `SpaceBeforeAssignmentOperators`, but the formatted 
code may be inconsistent...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66332



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

[PATCH] D66332: [clang-format] Fix the bug that joins template closer and > or >>

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

Quuxplusone added a comment.

In D66332#1633749 , @owenpan wrote:

> Do you have `SpaceBeforeAssignmentOperators` off?


Yes; I mean, that's what I tested in order to produce the buggy behavior. I 
don't believe anyone should be using that option in production, though, and 
would totally support a patch to remove it completely because it can't possibly 
be doing anyone any good.  It feels like someone meant to implement 
`SpacesAroundAssignmentOperators` but then got the name wrong, and then someone 
fixed the behavior to match the name, and at this point it's just completely 
broken. :)

> The documentation 
> 
>  should be fixed.

Also agreed.

Are there any other clang-format options that might lead to a lack-of-space 
before `>`, `>=`, `==`, or `=`? I brought up `enable_if_t=0` because 
that specifically is a construction I've run into in my own code, even though 
I've never tried to clang-format it, let alone have it formatted incorrectly.

(Clang-format probably does not promise to preserve the AST of pathological 
input like `i++ ++;` or C++03-specific input such as `A >` or `A< ::B>`.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66332



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

[PATCH] D66332: [clang-format] Fix the bug that joins template closer and > or >>

[Owen Pan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGac67414618df: [clang-format] Fix the bug that joins template 
closer and  or  (authored by owenpan).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66332

Files:
  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
@@ -6618,7 +6618,10 @@
   EXPECT_EQ("auto x = [] { A>> a; };",
 format("auto x=[]{A >> a;};", getGoogleStyle()));
 
-  verifyFormat("A> a;", getChromiumStyle(FormatStyle::LK_Cpp));
+  verifyFormat("A> a;", getChromiumStyle(FormatStyle::LK_Cpp));
+
+  verifyFormat("int i = a<1> >> 1;");
+  verifyFormat("bool b = a<1> > 1;");
 
   verifyFormat("test >> a >> b;");
   verifyFormat("test << a >> b;");
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -919,6 +919,8 @@
 case tok::greater:
   if (Style.Language != FormatStyle::LK_TextProto)
 Tok->Type = TT_BinaryOperator;
+  if (Tok->Previous && Tok->Previous->is(TT_TemplateCloser))
+Tok->SpacesRequiredBefore = 1;
   break;
 case tok::kw_operator:
   if (Style.Language == FormatStyle::LK_TextProto ||


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6618,7 +6618,10 @@
   EXPECT_EQ("auto x = [] { A>> a; };",
 format("auto x=[]{A >> a;};", getGoogleStyle()));
 
-  verifyFormat("A> a;", getChromiumStyle(FormatStyle::LK_Cpp));
+  verifyFormat("A> a;", getChromiumStyle(FormatStyle::LK_Cpp));
+
+  verifyFormat("int i = a<1> >> 1;");
+  verifyFormat("bool b = a<1> > 1;");
 
   verifyFormat("test >> a >> b;");
   verifyFormat("test << a >> b;");
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -919,6 +919,8 @@
 case tok::greater:
   if (Style.Language != FormatStyle::LK_TextProto)
 Tok->Type = TT_BinaryOperator;
+  if (Tok->Previous && Tok->Previous->is(TT_TemplateCloser))
+Tok->SpacesRequiredBefore = 1;
   break;
 case tok::kw_operator:
   if (Style.Language == FormatStyle::LK_TextProto ||
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66332: [clang-format] Fix the bug that joins template closer and > or >>

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

In D66332#1633448 , @Quuxplusone wrote:

> Drive-by observation: My experiments with 
> https://zed0.co.uk/clang-format-configurator/ show that there is a similar 
> bug where clang-format accidentally produces `>=` via reformatting of 
> `template` `=0>`, `pi` `=3;`, and so on. Is it 
> possible to fix that bug as part of this patch, and/or would you be 
> interested in patching that bug as a follow-up to this one?


Do you have `SpaceBeforeAssignmentOperators` off? I am more than happy to fix 
it as a follow-up, but the current behavior of `SpaceBeforeAssignmentOperators` 
is:
`true`:

  int a = 5;
  a += 42;

`false`:

  int a= 5;
  a+= 42;

It's weird that there is a space after the assignment operators regardless.

The documentation 

 should be fixed.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66332



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

[PATCH] D66332: [clang-format] Fix the bug that joins template closer and > or >>

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

Quuxplusone added a comment.

Drive-by observation: My experiments with 
https://zed0.co.uk/clang-format-configurator/ show that there is a similar bug 
where clang-format accidentally produces `>=` via reformatting of 
`template` `=0>`, `pi` `=3;`, and so on. Is it 
possible to fix that bug as part of this patch, and/or would you be interested 
in patching that bug as a follow-up to this one?


Repository:
  rC Clang

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

https://reviews.llvm.org/D66332



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

[PATCH] D66332: [clang-format] Fix the bug that joins template closer and > or >>

[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/D66332/new/

https://reviews.llvm.org/D66332



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

[PATCH] D66332: [clang-format] Fix the bug that joins template closer and > or >>

[Owen Pan via Phabricator via cfe-commits]

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

Also fixes a buggy test case.

See PR42404


Repository:
  rC Clang

https://reviews.llvm.org/D66332

Files:
  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
@@ -6618,7 +6618,10 @@
   EXPECT_EQ("auto x = [] { A>> a; };",
 format("auto x=[]{A >> a;};", getGoogleStyle()));
 
-  verifyFormat("A> a;", getChromiumStyle(FormatStyle::LK_Cpp));
+  verifyFormat("A> a;", getChromiumStyle(FormatStyle::LK_Cpp));
+
+  verifyFormat("int i = a<1> >> 1;");
+  verifyFormat("bool b = a<1> > 1;");
 
   verifyFormat("test >> a >> b;");
   verifyFormat("test << a >> b;");
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -919,6 +919,8 @@
 case tok::greater:
   if (Style.Language != FormatStyle::LK_TextProto)
 Tok->Type = TT_BinaryOperator;
+  if (Tok->Previous && Tok->Previous->is(TT_TemplateCloser))
+Tok->SpacesRequiredBefore = 1;
   break;
 case tok::kw_operator:
   if (Style.Language == FormatStyle::LK_TextProto ||


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6618,7 +6618,10 @@
   EXPECT_EQ("auto x = [] { A>> a; };",
 format("auto x=[]{A >> a;};", getGoogleStyle()));
 
-  verifyFormat("A> a;", getChromiumStyle(FormatStyle::LK_Cpp));
+  verifyFormat("A> a;", getChromiumStyle(FormatStyle::LK_Cpp));
+
+  verifyFormat("int i = a<1> >> 1;");
+  verifyFormat("bool b = a<1> > 1;");
 
   verifyFormat("test >> a >> b;");
   verifyFormat("test << a >> b;");
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -919,6 +919,8 @@
 case tok::greater:
   if (Style.Language != FormatStyle::LK_TextProto)
 Tok->Type = TT_BinaryOperator;
+  if (Tok->Previous && Tok->Previous->is(TT_TemplateCloser))
+Tok->SpacesRequiredBefore = 1;
   break;
 case tok::kw_operator:
   if (Style.Language == FormatStyle::LK_TextProto ||
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits