[PATCH] D59627: [clang-format] Keep protobuf "package" statement on one line

[Donald Chai via Phabricator via cfe-commits]

dchai created this revision.
dchai added a reviewer: sammccall.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Top-level "package" and "import" statements should generally be kept on one
line, for all languages.


Repository:
  rC Clang

https://reviews.llvm.org/D59627

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestProto.cpp


Index: unittests/Format/FormatTestProto.cpp
===
--- unittests/Format/FormatTestProto.cpp
+++ unittests/Format/FormatTestProto.cpp
@@ -387,6 +387,12 @@
"};");
 }
 
+TEST_F(FormatTestProto, DoesntWrapPackageStatements) {
+  verifyFormat(
+  "package"
+  " some.really.long.package.that.exceeds.the.column.limit;");
+}
+
 TEST_F(FormatTestProto, FormatsService) {
   verifyFormat("service SearchService {\n"
"  rpc Search(SearchRequest) returns (SearchResponse) {\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1073,10 +1073,11 @@
   return LT_ImportStatement;
 }
 
-// In .proto files, top-level options are very similar to import statements
-// and should not be line-wrapped.
+// In .proto files, top-level options and package statements are very
+// similar to import statements and should not be line-wrapped.
 if (Style.Language == FormatStyle::LK_Proto && Line.Level == 0 &&
-CurrentToken->is(Keywords.kw_option)) {
+(CurrentToken->is(Keywords.kw_option) ||
+ CurrentToken->is(Keywords.kw_package))) {
   next();
   if (CurrentToken && CurrentToken->is(tok::identifier))
 return LT_ImportStatement;


Index: unittests/Format/FormatTestProto.cpp
===
--- unittests/Format/FormatTestProto.cpp
+++ unittests/Format/FormatTestProto.cpp
@@ -387,6 +387,12 @@
"};");
 }
 
+TEST_F(FormatTestProto, DoesntWrapPackageStatements) {
+  verifyFormat(
+  "package"
+  " some.really.long.package.that.exceeds.the.column.limit;");
+}
+
 TEST_F(FormatTestProto, FormatsService) {
   verifyFormat("service SearchService {\n"
"  rpc Search(SearchRequest) returns (SearchResponse) {\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1073,10 +1073,11 @@
   return LT_ImportStatement;
 }
 
-// In .proto files, top-level options are very similar to import statements
-// and should not be line-wrapped.
+// In .proto files, top-level options and package statements are very
+// similar to import statements and should not be line-wrapped.
 if (Style.Language == FormatStyle::LK_Proto && Line.Level == 0 &&
-CurrentToken->is(Keywords.kw_option)) {
+(CurrentToken->is(Keywords.kw_option) ||
+ CurrentToken->is(Keywords.kw_package))) {
   next();
   if (CurrentToken && CurrentToken->is(tok::identifier))
 return LT_ImportStatement;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59629: [clang-format] correctly format protobuf fields named "enum".

[Donald Chai via Phabricator via cfe-commits]

dchai created this revision.
dchai added a reviewer: krasimir.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Similar to TypeScript, "enum" is not a reserved word.


Repository:
  rC Clang

https://reviews.llvm.org/D59629

Files:
  lib/Format/UnwrappedLineParser.cpp
  unittests/Format/FormatTestProto.cpp


Index: unittests/Format/FormatTestProto.cpp
===
--- unittests/Format/FormatTestProto.cpp
+++ unittests/Format/FormatTestProto.cpp
@@ -107,6 +107,12 @@
"};");
 }
 
+TEST_F(FormatTestProto, EnumAsFieldName) {
+  verifyFormat("message SomeMessage {\n"
+   "  required int32 enum = 1;\n"
+   "}");
+}
+
 TEST_F(FormatTestProto, UnderstandsReturns) {
   verifyFormat("rpc Search(SearchRequest) returns (SearchResponse);");
 }
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -2018,6 +2018,10 @@
   FormatTok->isOneOf(tok::colon, tok::question))
 return false;
 
+  // In protobuf, "enum" can be used as a field name.
+  if (Style.Language == FormatStyle::LK_Proto && FormatTok->is(tok::equal))
+return false;
+
   // Eat up enum class ...
   if (FormatTok->Tok.is(tok::kw_class) || FormatTok->Tok.is(tok::kw_struct))
 nextToken();


Index: unittests/Format/FormatTestProto.cpp
===
--- unittests/Format/FormatTestProto.cpp
+++ unittests/Format/FormatTestProto.cpp
@@ -107,6 +107,12 @@
"};");
 }
 
+TEST_F(FormatTestProto, EnumAsFieldName) {
+  verifyFormat("message SomeMessage {\n"
+   "  required int32 enum = 1;\n"
+   "}");
+}
+
 TEST_F(FormatTestProto, UnderstandsReturns) {
   verifyFormat("rpc Search(SearchRequest) returns (SearchResponse);");
 }
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -2018,6 +2018,10 @@
   FormatTok->isOneOf(tok::colon, tok::question))
 return false;
 
+  // In protobuf, "enum" can be used as a field name.
+  if (Style.Language == FormatStyle::LK_Proto && FormatTok->is(tok::equal))
+return false;
+
   // Eat up enum class ...
   if (FormatTok->Tok.is(tok::kw_class) || FormatTok->Tok.is(tok::kw_struct))
 nextToken();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59627: [clang-format] Keep protobuf "package" statement on one line

[Donald Chai via Phabricator via cfe-commits]

dchai updated this revision to Diff 191734.
dchai added a comment.

Use "isOneOf" instead of two calls to "is".


Repository:
  rC Clang

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

https://reviews.llvm.org/D59627

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestProto.cpp


Index: unittests/Format/FormatTestProto.cpp
===
--- unittests/Format/FormatTestProto.cpp
+++ unittests/Format/FormatTestProto.cpp
@@ -387,6 +387,12 @@
"};");
 }
 
+TEST_F(FormatTestProto, DoesntWrapPackageStatements) {
+  verifyFormat(
+  "package"
+  " some.really.long.package.that.exceeds.the.column.limit;");
+}
+
 TEST_F(FormatTestProto, FormatsService) {
   verifyFormat("service SearchService {\n"
"  rpc Search(SearchRequest) returns (SearchResponse) {\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1073,10 +1073,10 @@
   return LT_ImportStatement;
 }
 
-// In .proto files, top-level options are very similar to import statements
-// and should not be line-wrapped.
+// In .proto files, top-level options and package statements are very
+// similar to import statements and should not be line-wrapped.
 if (Style.Language == FormatStyle::LK_Proto && Line.Level == 0 &&
-CurrentToken->is(Keywords.kw_option)) {
+CurrentToken->isOneOf(Keywords.kw_option, Keywords.kw_package)) {
   next();
   if (CurrentToken && CurrentToken->is(tok::identifier))
 return LT_ImportStatement;


Index: unittests/Format/FormatTestProto.cpp
===
--- unittests/Format/FormatTestProto.cpp
+++ unittests/Format/FormatTestProto.cpp
@@ -387,6 +387,12 @@
"};");
 }
 
+TEST_F(FormatTestProto, DoesntWrapPackageStatements) {
+  verifyFormat(
+  "package"
+  " some.really.long.package.that.exceeds.the.column.limit;");
+}
+
 TEST_F(FormatTestProto, FormatsService) {
   verifyFormat("service SearchService {\n"
"  rpc Search(SearchRequest) returns (SearchResponse) {\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1073,10 +1073,10 @@
   return LT_ImportStatement;
 }
 
-// In .proto files, top-level options are very similar to import statements
-// and should not be line-wrapped.
+// In .proto files, top-level options and package statements are very
+// similar to import statements and should not be line-wrapped.
 if (Style.Language == FormatStyle::LK_Proto && Line.Level == 0 &&
-CurrentToken->is(Keywords.kw_option)) {
+CurrentToken->isOneOf(Keywords.kw_option, Keywords.kw_package)) {
   next();
   if (CurrentToken && CurrentToken->is(tok::identifier))
 return LT_ImportStatement;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59629: [clang-format] correctly format protobuf fields named "enum".

[Donald Chai via Phabricator via cfe-commits]

dchai added a comment.

I don't have commit access; can someone please merge this on my behalf?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59629



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

[PATCH] D59627: [clang-format] Keep protobuf "package" statement on one line

[Donald Chai via Phabricator via cfe-commits]

dchai added a comment.

Just to confirm, the regression is in the number of spaces before a trailing 
comment?

Before (2 spaces):

  package foo.bar;  // foo.bar package

After (1 space):

  package foo.bar; // foo.bar package

Top-level options are handled the same way.  I'll see if I can address these 
both.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59627



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

[PATCH] D60541: [clang-format] Use SpacesBeforeTrailingComments for "option" directive

[Donald Chai via Phabricator via cfe-commits]

dchai created this revision.
dchai added a reviewer: krasimir.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

AnnotatingParser::next() is needed to implicitly set TT_BlockComment
versus TT_LineComment.  On most other paths through
AnnotatingParser::parseLine(), all tokens are consumed to achieve that.
This change updates one place where this wasn't done.


Repository:
  rC Clang

https://reviews.llvm.org/D60541

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestProto.cpp


Index: unittests/Format/FormatTestProto.cpp
===
--- unittests/Format/FormatTestProto.cpp
+++ unittests/Format/FormatTestProto.cpp
@@ -193,6 +193,10 @@
  "\"some.really.long.package.that.exceeds.the.column.limit\";"));
 }
 
+TEST_F(FormatTestProto, TrailingCommentAfterFileOption) {
+  verifyFormat("option java_package = \"foo.pkg\";  // comment\n");
+}
+
 TEST_F(FormatTestProto, FormatsOptions) {
   verifyFormat("option (MyProto.options) = {\n"
"  field_a: OK\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1124,8 +1124,11 @@
 if (Style.Language == FormatStyle::LK_Proto && Line.Level == 0 &&
 CurrentToken->is(Keywords.kw_option)) {
   next();
-  if (CurrentToken && CurrentToken->is(tok::identifier))
+  if (CurrentToken && CurrentToken->is(tok::identifier)) {
+while (CurrentToken)
+  next();
 return LT_ImportStatement;
+  }
 }
 
 bool KeywordVirtualFound = false;


Index: unittests/Format/FormatTestProto.cpp
===
--- unittests/Format/FormatTestProto.cpp
+++ unittests/Format/FormatTestProto.cpp
@@ -193,6 +193,10 @@
  "\"some.really.long.package.that.exceeds.the.column.limit\";"));
 }
 
+TEST_F(FormatTestProto, TrailingCommentAfterFileOption) {
+  verifyFormat("option java_package = \"foo.pkg\";  // comment\n");
+}
+
 TEST_F(FormatTestProto, FormatsOptions) {
   verifyFormat("option (MyProto.options) = {\n"
"  field_a: OK\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1124,8 +1124,11 @@
 if (Style.Language == FormatStyle::LK_Proto && Line.Level == 0 &&
 CurrentToken->is(Keywords.kw_option)) {
   next();
-  if (CurrentToken && CurrentToken->is(tok::identifier))
+  if (CurrentToken && CurrentToken->is(tok::identifier)) {
+while (CurrentToken)
+  next();
 return LT_ImportStatement;
+  }
 }
 
 bool KeywordVirtualFound = false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60541: [clang-format] Use SpacesBeforeTrailingComments for "option" directive

[Donald Chai via Phabricator via cfe-commits]

dchai removed a subscriber: cfe-commits.
dchai added a comment.

Can someone please merge this for me?  I don't have commit access.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60541



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

[PATCH] D60661: Revert "Revert "[clang-format] Keep protobuf "package" statement on one line""

[Donald Chai via Phabricator via cfe-commits]

dchai created this revision.
dchai added a reviewer: krasimir.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Top-level "package" and "import" statements should generally be kept on
one line, for all languages.



This reverts commit rL356912 .
The regression from rL356835  was fixed via 
rC358275 .


Repository:
  rC Clang

https://reviews.llvm.org/D60661

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestProto.cpp


Index: unittests/Format/FormatTestProto.cpp
===
--- unittests/Format/FormatTestProto.cpp
+++ unittests/Format/FormatTestProto.cpp
@@ -397,6 +397,16 @@
"};");
 }
 
+TEST_F(FormatTestProto, DoesntWrapPackageStatements) {
+  verifyFormat(
+  "package"
+  " some.really.long.package.that.exceeds.the.column.limit;");
+}
+
+TEST_F(FormatTestProto, TrailingCommentAfterPackage) {
+  verifyFormat("package foo.pkg;  // comment\n");
+}
+
 TEST_F(FormatTestProto, FormatsService) {
   verifyFormat("service SearchService {\n"
"  rpc Search(SearchRequest) returns (SearchResponse) {\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1119,10 +1119,10 @@
   return LT_ImportStatement;
 }
 
-// In .proto files, top-level options are very similar to import statements
-// and should not be line-wrapped.
+// In .proto files, top-level options and package statements are very
+// similar to import statements and should not be line-wrapped.
 if (Style.Language == FormatStyle::LK_Proto && Line.Level == 0 &&
-CurrentToken->is(Keywords.kw_option)) {
+CurrentToken->isOneOf(Keywords.kw_option, Keywords.kw_package)) {
   next();
   if (CurrentToken && CurrentToken->is(tok::identifier)) {
 while (CurrentToken)


Index: unittests/Format/FormatTestProto.cpp
===
--- unittests/Format/FormatTestProto.cpp
+++ unittests/Format/FormatTestProto.cpp
@@ -397,6 +397,16 @@
"};");
 }
 
+TEST_F(FormatTestProto, DoesntWrapPackageStatements) {
+  verifyFormat(
+  "package"
+  " some.really.long.package.that.exceeds.the.column.limit;");
+}
+
+TEST_F(FormatTestProto, TrailingCommentAfterPackage) {
+  verifyFormat("package foo.pkg;  // comment\n");
+}
+
 TEST_F(FormatTestProto, FormatsService) {
   verifyFormat("service SearchService {\n"
"  rpc Search(SearchRequest) returns (SearchResponse) {\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1119,10 +1119,10 @@
   return LT_ImportStatement;
 }
 
-// In .proto files, top-level options are very similar to import statements
-// and should not be line-wrapped.
+// In .proto files, top-level options and package statements are very
+// similar to import statements and should not be line-wrapped.
 if (Style.Language == FormatStyle::LK_Proto && Line.Level == 0 &&
-CurrentToken->is(Keywords.kw_option)) {
+CurrentToken->isOneOf(Keywords.kw_option, Keywords.kw_package)) {
   next();
   if (CurrentToken && CurrentToken->is(tok::identifier)) {
 while (CurrentToken)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60661: Revert "Revert "[clang-format] Keep protobuf "package" statement on one line""

[Donald Chai via Phabricator via cfe-commits]

dchai added a comment.

Friendly ping.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60661



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

[PATCH] D60661: Revert "Revert "[clang-format] Keep protobuf "package" statement on one line""

[Donald Chai via Phabricator via cfe-commits]

dchai planned changes to this revision.
dchai added a comment.

Hi, can someone please merge this on my behalf?  I don't have commit access.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60661



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

[PATCH] D60661: Revert "Revert "[clang-format] Keep protobuf "package" statement on one line""

[Donald Chai via Phabricator via cfe-commits]

dchai updated this revision to Diff 198585.
dchai added a comment.
This revision is now accepted and ready to land.

no-op to try to get rid of "Changes Planned" state


Repository:
  rC Clang

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

https://reviews.llvm.org/D60661

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestProto.cpp


Index: unittests/Format/FormatTestProto.cpp
===
--- unittests/Format/FormatTestProto.cpp
+++ unittests/Format/FormatTestProto.cpp
@@ -397,6 +397,16 @@
"};");
 }
 
+TEST_F(FormatTestProto, DoesntWrapPackageStatements) {
+  verifyFormat(
+  "package"
+  " some.really.long.package.that.exceeds.the.column.limit;");
+}
+
+TEST_F(FormatTestProto, TrailingCommentAfterPackage) {
+  verifyFormat("package foo.pkg;  // comment\n");
+}
+
 TEST_F(FormatTestProto, FormatsService) {
   verifyFormat("service SearchService {\n"
"  rpc Search(SearchRequest) returns (SearchResponse) {\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1119,10 +1119,10 @@
   return LT_ImportStatement;
 }
 
-// In .proto files, top-level options are very similar to import statements
-// and should not be line-wrapped.
+// In .proto files, top-level options and package statements are very
+// similar to import statements and should not be line-wrapped.
 if (Style.Language == FormatStyle::LK_Proto && Line.Level == 0 &&
-CurrentToken->is(Keywords.kw_option)) {
+CurrentToken->isOneOf(Keywords.kw_option, Keywords.kw_package)) {
   next();
   if (CurrentToken && CurrentToken->is(tok::identifier)) {
 while (CurrentToken)


Index: unittests/Format/FormatTestProto.cpp
===
--- unittests/Format/FormatTestProto.cpp
+++ unittests/Format/FormatTestProto.cpp
@@ -397,6 +397,16 @@
"};");
 }
 
+TEST_F(FormatTestProto, DoesntWrapPackageStatements) {
+  verifyFormat(
+  "package"
+  " some.really.long.package.that.exceeds.the.column.limit;");
+}
+
+TEST_F(FormatTestProto, TrailingCommentAfterPackage) {
+  verifyFormat("package foo.pkg;  // comment\n");
+}
+
 TEST_F(FormatTestProto, FormatsService) {
   verifyFormat("service SearchService {\n"
"  rpc Search(SearchRequest) returns (SearchResponse) {\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -1119,10 +1119,10 @@
   return LT_ImportStatement;
 }
 
-// In .proto files, top-level options are very similar to import statements
-// and should not be line-wrapped.
+// In .proto files, top-level options and package statements are very
+// similar to import statements and should not be line-wrapped.
 if (Style.Language == FormatStyle::LK_Proto && Line.Level == 0 &&
-CurrentToken->is(Keywords.kw_option)) {
+CurrentToken->isOneOf(Keywords.kw_option, Keywords.kw_package)) {
   next();
   if (CurrentToken && CurrentToken->is(tok::identifier)) {
 while (CurrentToken)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits