[PATCH] D27377: clang-format: Support the Java 8 'default' method modifier
MyDeveloperDay commandeered this revision. MyDeveloperDay edited reviewers, added: lhchavez; removed: MyDeveloperDay. MyDeveloperDay added a comment. This revision is no needed, the test will pass without any additional code changes CHANGES SINCE LAST ACTION https://reviews.llvm.org/D27377/new/ https://reviews.llvm.org/D27377 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27377: clang-format: Support the Java 8 'default' method modifier
MyDeveloperDay added a comment. While reviewing old revisions either not landed or unreviewed I stumbled across this accepted review. Looking at the current code I think its not needed anymore. Perhaps if we merge your test in with the current trunk we can determine if this is still needed CHANGES SINCE LAST ACTION https://reviews.llvm.org/D27377/new/ https://reviews.llvm.org/D27377 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27377: clang-format: Support the Java 8 'default' method modifier
djasper accepted this revision. djasper added a comment. This revision is now accepted and ready to land. Looks good. Thank you! https://reviews.llvm.org/D27377 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27377: clang-format: Support the Java 8 'default' method modifier
lhchavez marked 3 inline comments as done. lhchavez added inline comments. Comment at: lib/Format/UnwrappedLineParser.cpp:870 +} +// 'default' can appear in a Java 8 declaration. Parse it as such. +break; djasper wrote: > Is there a test case that hits this codepath? IIUC, it would need to have a > "default" of a declaration that's not at the start of the line. Added two more test cases. https://reviews.llvm.org/D27377 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27377: clang-format: Support the Java 8 'default' method modifier
lhchavez updated this revision to Diff 81280.
lhchavez added a comment.
Addressed feedback
https://reviews.llvm.org/D27377
Files:
lib/Format/UnwrappedLineParser.cpp
unittests/Format/FormatTestJava.cpp
Index: unittests/Format/FormatTestJava.cpp
===
--- unittests/Format/FormatTestJava.cpp
+++ unittests/Format/FormatTestJava.cpp
@@ -515,5 +515,22 @@
" void f() {}"));
}
+TEST_F(FormatTestJava, UnderstandsDefaultModifier) {
+ verifyFormat("class SomeClass {\n"
+ " default void f() {}\n"
+ " public default void g() {}\n"
+ " default public void h() {}\n"
+ " int i() {\n"
+ "switch (0) {\n"
+ " case 0:\n"
+ "break;\n"
+ " default:\n"
+ "break;\n"
+ "}\n"
+ " }\n"
+ "}",
+ getGoogleStyle(FormatStyle::LK_Java));
+}
+
} // end namespace tooling
} // end namespace clang
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -294,6 +294,11 @@
addUnwrappedLine();
break;
case tok::kw_default:
+ if (Style.Language == FormatStyle::LK_Java && Line->MustBeDeclaration) {
+parseStructuralElement();
+break;
+ }
+ // fallthrough
case tok::kw_case:
if (!SwitchLabelEncountered &&
(Style.IndentCaseLabels || (Line->InPPDirective && Line->Level ==
1)))
@@ -853,6 +858,8 @@
parseSwitch();
return;
case tok::kw_default:
+if (Style.Language == FormatStyle::LK_Java && Line->MustBeDeclaration)
+ break;
nextToken();
parseLabel();
return;
Index: unittests/Format/FormatTestJava.cpp
===
--- unittests/Format/FormatTestJava.cpp
+++ unittests/Format/FormatTestJava.cpp
@@ -515,5 +515,22 @@
" void f() {}"));
}
+TEST_F(FormatTestJava, UnderstandsDefaultModifier) {
+ verifyFormat("class SomeClass {\n"
+ " default void f() {}\n"
+ " public default void g() {}\n"
+ " default public void h() {}\n"
+ " int i() {\n"
+ "switch (0) {\n"
+ " case 0:\n"
+ "break;\n"
+ " default:\n"
+ "break;\n"
+ "}\n"
+ " }\n"
+ "}",
+ getGoogleStyle(FormatStyle::LK_Java));
+}
+
} // end namespace tooling
} // end namespace clang
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -294,6 +294,11 @@
addUnwrappedLine();
break;
case tok::kw_default:
+ if (Style.Language == FormatStyle::LK_Java && Line->MustBeDeclaration) {
+parseStructuralElement();
+break;
+ }
+ // fallthrough
case tok::kw_case:
if (!SwitchLabelEncountered &&
(Style.IndentCaseLabels || (Line->InPPDirective && Line->Level == 1)))
@@ -853,6 +858,8 @@
parseSwitch();
return;
case tok::kw_default:
+if (Style.Language == FormatStyle::LK_Java && Line->MustBeDeclaration)
+ break;
nextToken();
parseLabel();
return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27377: clang-format: Support the Java 8 'default' method modifier
djasper added inline comments.
Comment at: lib/Format/UnwrappedLineParser.cpp:297
case tok::kw_default:
+ if (Style.Language != FormatStyle::LK_Java || !Line->MustBeDeclaration) {
+if (!SwitchLabelEncountered &&
Same as below.
Comment at: lib/Format/UnwrappedLineParser.cpp:865
case tok::kw_default:
-nextToken();
-parseLabel();
-return;
+if (Style.Language != FormatStyle::LK_Java || !Line->MustBeDeclaration) {
+ nextToken();
Change the order here. I.e.
if (Style.Language == FormatStyle::LK_Java && Line->MustBeDeclaration)
break;
...
I think then you almost don't even need the comment.
Comment at: lib/Format/UnwrappedLineParser.cpp:870
+}
+// 'default' can appear in a Java 8 declaration. Parse it as such.
+break;
Is there a test case that hits this codepath? IIUC, it would need to have a
"default" of a declaration that's not at the start of the line.
https://reviews.llvm.org/D27377
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27377: clang-format: Support the Java 8 'default' method modifier
lodato resigned from this revision. lodato removed a reviewer: lodato. lodato added a comment. I know nothing about the C++ code. I only know the git-clang-format script. https://reviews.llvm.org/D27377 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27377: clang-format: Support the Java 8 'default' method modifier
lhchavez added a comment. Gentle ping? https://reviews.llvm.org/D27377 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27377: clang-format: Support the Java 8 'default' method modifier
srhines added a comment. Looks good, but I I want to make sure that someone else more familiar with this is ok with it too. Thanks. https://reviews.llvm.org/D27377 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27377: clang-format: Support the Java 8 'default' method modifier
lhchavez created this revision.
lhchavez added a reviewer: djasper.
lhchavez added subscribers: srhines, cfe-commits.
Herald added a subscriber: klimek.
Java 8 introduced the use of using the 'default' keyword as modifier in
interface method declarations[1]. Previously it was being parsed as
being part of a label, which put the parser into a very weird state it
could not get out of.
This change adds support for 'default' by treating it as a normal
identifier in Java when the parser is expecting a declaration.
1: http://docs.oracle.com/javase/tutorial/java/IandI/defaultmethods.html
https://reviews.llvm.org/D27377
Files:
lib/Format/UnwrappedLineParser.cpp
unittests/Format/FormatTestJava.cpp
Index: unittests/Format/FormatTestJava.cpp
===
--- unittests/Format/FormatTestJava.cpp
+++ unittests/Format/FormatTestJava.cpp
@@ -515,5 +515,20 @@
" void f() {}"));
}
+TEST_F(FormatTestJava, UnderstandsDefaultModifier) {
+ verifyFormat("class SomeClass {\n"
+ " default void f() {}\n"
+ " int g() {\n"
+ "switch (0) {\n"
+ " case 0:\n"
+ "break;\n"
+ " default:\n"
+ "break;\n"
+ "}\n"
+ " }\n"
+ "}",
+ getGoogleStyle(FormatStyle::LK_Java));
+}
+
} // end namespace tooling
} // end namespace clang
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -294,6 +294,15 @@
addUnwrappedLine();
break;
case tok::kw_default:
+ if (Style.Language != FormatStyle::LK_Java || !Line->MustBeDeclaration) {
+if (!SwitchLabelEncountered &&
+(Style.IndentCaseLabels ||
+ (Line->InPPDirective && Line->Level == 1)))
+ ++Line->Level;
+SwitchLabelEncountered = true;
+ }
+ parseStructuralElement();
+ break;
case tok::kw_case:
if (!SwitchLabelEncountered &&
(Style.IndentCaseLabels || (Line->InPPDirective && Line->Level ==
1)))
@@ -853,9 +862,13 @@
parseSwitch();
return;
case tok::kw_default:
-nextToken();
-parseLabel();
-return;
+if (Style.Language != FormatStyle::LK_Java || !Line->MustBeDeclaration) {
+ nextToken();
+ parseLabel();
+ return;
+}
+// 'default' can appear in a Java 8 declaration. Parse it as such.
+break;
case tok::kw_case:
parseCaseLabel();
return;
Index: unittests/Format/FormatTestJava.cpp
===
--- unittests/Format/FormatTestJava.cpp
+++ unittests/Format/FormatTestJava.cpp
@@ -515,5 +515,20 @@
" void f() {}"));
}
+TEST_F(FormatTestJava, UnderstandsDefaultModifier) {
+ verifyFormat("class SomeClass {\n"
+ " default void f() {}\n"
+ " int g() {\n"
+ "switch (0) {\n"
+ " case 0:\n"
+ "break;\n"
+ " default:\n"
+ "break;\n"
+ "}\n"
+ " }\n"
+ "}",
+ getGoogleStyle(FormatStyle::LK_Java));
+}
+
} // end namespace tooling
} // end namespace clang
Index: lib/Format/UnwrappedLineParser.cpp
===
--- lib/Format/UnwrappedLineParser.cpp
+++ lib/Format/UnwrappedLineParser.cpp
@@ -294,6 +294,15 @@
addUnwrappedLine();
break;
case tok::kw_default:
+ if (Style.Language != FormatStyle::LK_Java || !Line->MustBeDeclaration) {
+if (!SwitchLabelEncountered &&
+(Style.IndentCaseLabels ||
+ (Line->InPPDirective && Line->Level == 1)))
+ ++Line->Level;
+SwitchLabelEncountered = true;
+ }
+ parseStructuralElement();
+ break;
case tok::kw_case:
if (!SwitchLabelEncountered &&
(Style.IndentCaseLabels || (Line->InPPDirective && Line->Level == 1)))
@@ -853,9 +862,13 @@
parseSwitch();
return;
case tok::kw_default:
-nextToken();
-parseLabel();
-return;
+if (Style.Language != FormatStyle::LK_Java || !Line->MustBeDeclaration) {
+ nextToken();
+ parseLabel();
+ return;
+}
+// 'default' can appear in a Java 8 declaration. Parse it as such.
+break;
case tok::kw_case:
parseCaseLabel();
return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
