[Format] Configure ASSIGN_OR_RETURN macros for Google style#200210
Open
jmr wants to merge 1 commit into
Open
Conversation
|
Hello @jmr 👋 Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.
Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description. Frequently asked questionsHow do I add reviewers? This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically. You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using What if there are no comments? If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers. Are any special GitHub settings required to contribute to LLVM? We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details. If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse. Thank you, |
|
@llvm/pr-subscribers-clang-format Author: Jesse Rosenstock (jmr) ChangesAdd macros that wrap assignment to GoogleStyle.
This is a resubmission of #169037 with fixes for the reported regressions caused by not having RETURN_IF_ERROR Macros defined. Reverts 7e51783 (Revert "[Format] Configure ASSIGN_OR_RETURN macros for Google style" (#186445)). Full diff: https://github.com/llvm/llvm-project/pull/200210.diff 3 Files Affected:
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index a29d62c99bb95..fbc514d36b9a2 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -2101,6 +2101,20 @@ FormatStyle getGoogleStyle(FormatStyle::LanguageKind Language) {
GoogleStyle.IncludeStyle.IncludeIsMainRegex = "([-_](test|unittest))?$";
GoogleStyle.IndentCaseLabels = true;
GoogleStyle.KeepEmptyLines.AtStartOfBlock = false;
+
+ GoogleStyle.Macros.push_back("ASSIGN_OR_RETURN(a, b)=a = (b)");
+ GoogleStyle.Macros.push_back(
+ "ASSIGN_OR_RETURN(a, b, c)=a = (b); if (x) return c");
+ GoogleStyle.Macros.push_back("RETURN_IF_ERROR(expr)=if (x) return expr");
+ GoogleStyle.Macros.push_back(
+ "ASSERT_OK_AND_ASSIGN(lhs, rexpr)=lhs = (rexpr)");
+ GoogleStyle.Macros.push_back("ABSL_ASSIGN_OR_RETURN(a, b)=a = (b)");
+ GoogleStyle.Macros.push_back(
+ "ABSL_ASSIGN_OR_RETURN(a, b, c)=a = (b); if (x) return c");
+ GoogleStyle.Macros.push_back("ABSL_RETURN_IF_ERROR(expr)=if (x) return expr");
+ GoogleStyle.Macros.push_back(
+ "ABSL_ASSERT_OK_AND_ASSIGN(lhs, rexpr)=lhs = (rexpr)");
+
GoogleStyle.ObjCBinPackProtocolList = FormatStyle::BPS_Never;
GoogleStyle.ObjCSpaceAfterProperty = false;
GoogleStyle.ObjCSpaceBeforeProtocolList = true;
diff --git a/clang/unittests/Format/ConfigParseTest.cpp b/clang/unittests/Format/ConfigParseTest.cpp
index eeaf5d3f66d96..20beed07b5075 100644
--- a/clang/unittests/Format/ConfigParseTest.cpp
+++ b/clang/unittests/Format/ConfigParseTest.cpp
@@ -1099,6 +1099,19 @@ TEST(ConfigParseTest, ParsesConfiguration) {
StatementAttributeLikeMacros,
std::vector<std::string>({"emit", "Q_EMIT"}));
+ Style.Macros.clear();
+ CHECK_PARSE("{Macros: [foo]}", Macros, std::vector<std::string>({"foo"}));
+ std::vector<std::string> GoogleMacros;
+ GoogleMacros.push_back("ASSIGN_OR_RETURN(a, b)=a = (b)");
+ GoogleMacros.push_back("ASSIGN_OR_RETURN(a, b, c)=a = (b); if (x) return c");
+ GoogleMacros.push_back("RETURN_IF_ERROR(expr)=if (x) return expr");
+ GoogleMacros.push_back("ASSERT_OK_AND_ASSIGN(lhs, rexpr)=lhs = (rexpr)");
+ GoogleMacros.push_back("ABSL_ASSIGN_OR_RETURN(a, b)=a = (b)");
+ GoogleMacros.push_back("ABSL_ASSIGN_OR_RETURN(a, b, c)=a = (b); if (x) return c");
+ GoogleMacros.push_back("ABSL_RETURN_IF_ERROR(expr)=if (x) return expr");
+ GoogleMacros.push_back("ABSL_ASSERT_OK_AND_ASSIGN(lhs, rexpr)=lhs = (rexpr)");
+ CHECK_PARSE("BasedOnStyle: Google", Macros, GoogleMacros);
+
Style.StatementMacros.clear();
CHECK_PARSE("StatementMacros: [QUNUSED]", StatementMacros,
std::vector<std::string>{"QUNUSED"});
@@ -1106,7 +1119,6 @@ TEST(ConfigParseTest, ParsesConfiguration) {
std::vector<std::string>({"QUNUSED", "QT_REQUIRE_VERSION"}));
CHECK_PARSE_LIST(JavaImportGroups);
- CHECK_PARSE_LIST(Macros);
CHECK_PARSE_LIST(MacrosSkippedByRemoveParentheses);
CHECK_PARSE_LIST(NamespaceMacros);
CHECK_PARSE_LIST(ObjCPropertyAttributeOrder);
diff --git a/clang/unittests/Format/FormatTestMacroExpansion.cpp b/clang/unittests/Format/FormatTestMacroExpansion.cpp
index d391fe3d715c3..2708173771bb0 100644
--- a/clang/unittests/Format/FormatTestMacroExpansion.cpp
+++ b/clang/unittests/Format/FormatTestMacroExpansion.cpp
@@ -58,10 +58,18 @@ TEST_F(FormatTestMacroExpansion, UnexpandConfiguredMacros) {
verifyFormat("ASSIGN_OR_RETURN(MySomewhatLongType *variable,\n"
" MySomewhatLongFunction(SomethingElse()));",
Style);
- verifyFormat("ASSIGN_OR_RETURN(MySomewhatLongType *variable,\n"
- " MySomewhatLongFunction(SomethingElse()), "
- "ReturnMe());",
- Style);
+ verifyFormat(
+ "ASSIGN_OR_RETURN(MySomewhatLongType *variable,\n"
+ " MySomewhatLongFunction(SomethingElse()), RetMe());",
+ Style);
+
+ verifyFormat(
+ "void f() {\n"
+ " ASSIGN_OR_RETURN(MySomewhatLongType* variable,\n"
+ " MySomewhatLongFunction(SomethingElse()));\n"
+ " ASSIGN_OR_RETURN(MySomewhatLongType* variable,\n"
+ " MySomewhatLongFunction(SomethingElse()), RetMe());",
+ getGoogleStyle());
verifyFormat(R"(
#define MACRO(a, b) ID(a + b)
@@ -301,6 +309,107 @@ TEST_F(FormatTestMacroExpansion, IndentChildrenWithinMacroCall) {
Style);
}
+TEST_F(FormatTestMacroExpansion, NestedMacrosInLambdas) {
+ // These are tests for regressions reported in
+ // https://github.com/llvm/llvm-project/pull/169037#issuecomment-4056423543
+ verifyFormat(
+ "void f() {\n"
+ " RETURN_IF_ERROR(Aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
+ " a,\n"
+ " []() {\n"
+ " if (z()) {\n"
+ " ASSIGN_OR_RETURN(w, Wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww());\n"
+ " }\n"
+ " }));\n"
+ "}",
+ getGoogleStyle());
+
+ verifyFormat(
+ "void g() {\n"
+ " RETURN_IF_ERROR(q(\n"
+ " w,\n"
+ " []() {\n"
+ " if (z()) {\n"
+ " ASSIGN_OR_RETURN(w, Wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww());\n"
+ " }\n"
+ " }));\n"
+ "}",
+ getGoogleStyle());
+
+ verifyFormat(
+ "void f() {\n"
+ " ABSL_RETURN_IF_ERROR(\n"
+ " Aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
+ " a,\n"
+ " []() {\n"
+ " if (z()) {\n"
+ " ABSL_ASSIGN_OR_RETURN(w, Wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww());\n"
+ " }\n"
+ " }));\n"
+ "}",
+ getGoogleStyle());
+
+ verifyFormat(
+ "void g() {\n"
+ " ABSL_RETURN_IF_ERROR(q(\n"
+ " w,\n"
+ " []() {\n"
+ " if (z()) {\n"
+ " ABSL_ASSIGN_OR_RETURN(w, Wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww());\n"
+ " }\n"
+ " }));\n"
+ "}",
+ getGoogleStyle());
+}
+
+TEST_F(FormatTestMacroExpansion, PredefinedGoogleMacros) {
+ verifyFormat(
+ "void f() {\n"
+ " ASSIGN_OR_RETURN(MySomewhatLongType* variable,\n"
+ " MySomewhatLongFunction(SomethingElse()));\n"
+ " ASSIGN_OR_RETURN(MySomewhatLongType* variable,\n"
+ " MySomewhatLongFunction(SomethingElse()), RetMe());\n"
+ "}",
+ getGoogleStyle());
+
+ verifyFormat(
+ "void f() {\n"
+ " ABSL_ASSIGN_OR_RETURN(MySomewhatLongType* variable,\n"
+ " MySomewhatLongFunction(SomethingElse()));\n"
+ " ABSL_ASSIGN_OR_RETURN(MySomewhatLongType* variable,\n"
+ " MySomewhatLongFunction(SomethingElse()), RetMe());\n"
+ "}",
+ getGoogleStyle());
+
+ verifyFormat(
+ "void f() {\n"
+ " RETURN_IF_ERROR(\n"
+ " MySomewhatLongFunction(SomethingElse(WithManyArguments, AndSomeMore)));\n"
+ "}",
+ getGoogleStyle());
+
+ verifyFormat(
+ "void f() {\n"
+ " ABSL_RETURN_IF_ERROR(\n"
+ " MySomewhatLongFunction(SomethingElse(WithManyArguments, AndSomeMore)));\n"
+ "}",
+ getGoogleStyle());
+
+ verifyFormat(
+ "void f() {\n"
+ " ASSERT_OK_AND_ASSIGN(MySomewhatLongType* variable,\n"
+ " MySomewhatLongFunction(SomethingElse()));\n"
+ "}",
+ getGoogleStyle());
+
+ verifyFormat(
+ "void f() {\n"
+ " ABSL_ASSERT_OK_AND_ASSIGN(MySomewhatLongType* variable,\n"
+ " MySomewhatLongFunction(SomethingElse()));\n"
+ "}",
+ getGoogleStyle());
+}
+
} // namespace
} // namespace test
} // namespace format
|
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp -- clang/lib/Format/Format.cpp clang/unittests/Format/ConfigParseTest.cpp clang/unittests/Format/FormatTestMacroExpansion.cpp --diff_from_common_commit
View the diff from clang-format here.diff --git a/clang/unittests/Format/ConfigParseTest.cpp b/clang/unittests/Format/ConfigParseTest.cpp
index 20beed07b..7f420eab6 100644
--- a/clang/unittests/Format/ConfigParseTest.cpp
+++ b/clang/unittests/Format/ConfigParseTest.cpp
@@ -1107,7 +1107,8 @@ TEST(ConfigParseTest, ParsesConfiguration) {
GoogleMacros.push_back("RETURN_IF_ERROR(expr)=if (x) return expr");
GoogleMacros.push_back("ASSERT_OK_AND_ASSIGN(lhs, rexpr)=lhs = (rexpr)");
GoogleMacros.push_back("ABSL_ASSIGN_OR_RETURN(a, b)=a = (b)");
- GoogleMacros.push_back("ABSL_ASSIGN_OR_RETURN(a, b, c)=a = (b); if (x) return c");
+ GoogleMacros.push_back(
+ "ABSL_ASSIGN_OR_RETURN(a, b, c)=a = (b); if (x) return c");
GoogleMacros.push_back("ABSL_RETURN_IF_ERROR(expr)=if (x) return expr");
GoogleMacros.push_back("ABSL_ASSERT_OK_AND_ASSIGN(lhs, rexpr)=lhs = (rexpr)");
CHECK_PARSE("BasedOnStyle: Google", Macros, GoogleMacros);
diff --git a/clang/unittests/Format/FormatTestMacroExpansion.cpp b/clang/unittests/Format/FormatTestMacroExpansion.cpp
index 270817377..41117b852 100644
--- a/clang/unittests/Format/FormatTestMacroExpansion.cpp
+++ b/clang/unittests/Format/FormatTestMacroExpansion.cpp
@@ -314,7 +314,9 @@ TEST_F(FormatTestMacroExpansion, NestedMacrosInLambdas) {
// https://github.com/llvm/llvm-project/pull/169037#issuecomment-4056423543
verifyFormat(
"void f() {\n"
- " RETURN_IF_ERROR(Aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
+ " "
+ "RETURN_IF_ERROR("
+ "Aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(\n"
" a,\n"
" []() {\n"
" if (z()) {\n"
@@ -343,23 +345,24 @@ TEST_F(FormatTestMacroExpansion, NestedMacrosInLambdas) {
" a,\n"
" []() {\n"
" if (z()) {\n"
- " ABSL_ASSIGN_OR_RETURN(w, Wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww());\n"
+ " ABSL_ASSIGN_OR_RETURN(w, "
+ "Wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww());\n"
" }\n"
" }));\n"
"}",
getGoogleStyle());
- verifyFormat(
- "void g() {\n"
- " ABSL_RETURN_IF_ERROR(q(\n"
- " w,\n"
- " []() {\n"
- " if (z()) {\n"
- " ABSL_ASSIGN_OR_RETURN(w, Wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww());\n"
- " }\n"
- " }));\n"
- "}",
- getGoogleStyle());
+ verifyFormat("void g() {\n"
+ " ABSL_RETURN_IF_ERROR(q(\n"
+ " w,\n"
+ " []() {\n"
+ " if (z()) {\n"
+ " ABSL_ASSIGN_OR_RETURN(w, "
+ "Wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww());\n"
+ " }\n"
+ " }));\n"
+ "}",
+ getGoogleStyle());
}
TEST_F(FormatTestMacroExpansion, PredefinedGoogleMacros) {
@@ -377,23 +380,24 @@ TEST_F(FormatTestMacroExpansion, PredefinedGoogleMacros) {
" ABSL_ASSIGN_OR_RETURN(MySomewhatLongType* variable,\n"
" MySomewhatLongFunction(SomethingElse()));\n"
" ABSL_ASSIGN_OR_RETURN(MySomewhatLongType* variable,\n"
- " MySomewhatLongFunction(SomethingElse()), RetMe());\n"
+ " MySomewhatLongFunction(SomethingElse()), "
+ "RetMe());\n"
"}",
getGoogleStyle());
- verifyFormat(
- "void f() {\n"
- " RETURN_IF_ERROR(\n"
- " MySomewhatLongFunction(SomethingElse(WithManyArguments, AndSomeMore)));\n"
- "}",
- getGoogleStyle());
+ verifyFormat("void f() {\n"
+ " RETURN_IF_ERROR(\n"
+ " MySomewhatLongFunction(SomethingElse(WithManyArguments, "
+ "AndSomeMore)));\n"
+ "}",
+ getGoogleStyle());
- verifyFormat(
- "void f() {\n"
- " ABSL_RETURN_IF_ERROR(\n"
- " MySomewhatLongFunction(SomethingElse(WithManyArguments, AndSomeMore)));\n"
- "}",
- getGoogleStyle());
+ verifyFormat("void f() {\n"
+ " ABSL_RETURN_IF_ERROR(\n"
+ " MySomewhatLongFunction(SomethingElse(WithManyArguments, "
+ "AndSomeMore)));\n"
+ "}",
+ getGoogleStyle());
verifyFormat(
"void f() {\n"
|
Add macros that wrap assignment to GoogleStyle. * ASSIGN_OR_RETURN/ABSL_ASSIGN_OR_RETURN * RETURN_IF_ERROR/ABSL_RETURN_IF_ERROR * ASSERT_OK_AND_ASSIGN/ABSL_ASSERT_OK_AND_ASSIGN `ASSIGN_OR_RETURN(T* p, f())` will be formatted as a multiplication without these macros definitions. This is a resubmission of llvm#169037 with fixes for the reported regressions caused by not having RETURN_IF_ERROR Macros defined. llvm#169037 (comment) Reverts 7e51783 (Revert "[Format] Configure ASSIGN_OR_RETURN macros for Google style" (llvm#186445)). Assisted-by: Gemini
Author
Confirmed. |
| GoogleStyle.IndentCaseLabels = true; | ||
| GoogleStyle.KeepEmptyLines.AtStartOfBlock = false; | ||
|
|
||
| GoogleStyle.Macros.push_back("ASSIGN_OR_RETURN(a, b)=a = (b)"); |
Contributor
There was a problem hiding this comment.
Please use one assignment, not a sequence of push_backs.
Author
There was a problem hiding this comment.
This is how https://github.com/llvm/llvm-project/pull/169037/changes, which was merged, did it.
| Style.StatementMacros.clear(); | ||
| CHECK_PARSE("StatementMacros: [QUNUSED]", StatementMacros, | ||
| std::vector<std::string>{"QUNUSED"}); | ||
| CHECK_PARSE("StatementMacros: [QUNUSED, QT_REQUIRE_VERSION]", StatementMacros, | ||
| std::vector<std::string>({"QUNUSED", "QT_REQUIRE_VERSION"})); | ||
|
|
||
| CHECK_PARSE_LIST(JavaImportGroups); | ||
| CHECK_PARSE_LIST(Macros); |
Contributor
There was a problem hiding this comment.
Is there a reason to remove this?
Author
There was a problem hiding this comment.
I copied from https://github.com/llvm/llvm-project/pull/169037/changes. I will have to look in more detail to figure out why it was done this way.
| "ABSL_ASSIGN_OR_RETURN(a, b, c)=a = (b); if (x) return c"); | ||
| GoogleMacros.push_back("ABSL_RETURN_IF_ERROR(expr)=if (x) return expr"); | ||
| GoogleMacros.push_back("ABSL_ASSERT_OK_AND_ASSIGN(lhs, rexpr)=lhs = (rexpr)"); | ||
| CHECK_PARSE("BasedOnStyle: Google", Macros, GoogleMacros); |
Contributor
There was a problem hiding this comment.
I'm questioning the reason for this test. That BasedOnStyle does work is tested elsewhere.
Author
There was a problem hiding this comment.
Again, modeled directly after https://github.com/llvm/llvm-project/pull/169037/changes
| @@ -301,6 +309,109 @@ TEST_F(FormatTestMacroExpansion, IndentChildrenWithinMacroCall) { | |||
| Style); | |||
| } | |||
|
|
|||
| // clang-format off | |||
Contributor
There was a problem hiding this comment.
It would be great if you can redo that just with a smaller column limit.
If you want to keep the one test case that is also acceptable, but limit that clang format off to the verifyFormat and not around all the test functions here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add macros that wrap assignment to GoogleStyle.
ASSIGN_OR_RETURN(T* p, f())will be formatted as a multiplication without these macros definitions.This is a resubmission of #169037 with fixes for the reported regressions caused by not having RETURN_IF_ERROR Macros defined.
#169037 (comment)
Reverts 7e51783 (Revert "[Format] Configure ASSIGN_OR_RETURN macros for Google style" (#186445)).