Diagnose the code with trailing comma in the function call. #125232
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Haojian Wu (hokein) ChangesFixes #125225 Full diff: https://github.com/llvm/llvm-project/pull/125232.diff 3 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticParseKinds.td b/clang/include/clang/Basic/DiagnosticParseKinds.td
index c513dab810d1f5..f116ef114bb361 100644
--- a/clang/include/clang/Basic/DiagnosticParseKinds.td
+++ b/clang/include/clang/Basic/DiagnosticParseKinds.td
@@ -703,6 +703,8 @@ def ext_consteval_if : ExtWarn<
def warn_cxx20_compat_consteval_if : Warning<
"consteval if is incompatible with C++ standards before C++23">,
InGroup<CXXPre23Compat>, DefaultIgnore;
+def err_extraneous_trailing_comma : Error<
+ "extraneous trailing comma">;
def ext_init_statement : ExtWarn<
"'%select{if|switch}0' initialization statements are a C++17 extension">,
diff --git a/clang/lib/Parse/ParseExpr.cpp b/clang/lib/Parse/ParseExpr.cpp
index aa8b3870a188c6..e31ef7d404a222 100644
--- a/clang/lib/Parse/ParseExpr.cpp
+++ b/clang/lib/Parse/ParseExpr.cpp
@@ -2237,6 +2237,9 @@ Parser::ParsePostfixExpressionSuffix(ExprResult LHS) {
if (PP.isCodeCompletionReached() && !CalledSignatureHelp)
RunSignatureHelp();
LHS = ExprError();
+ } else if (!HasError && HasTrailingComma) {
+ // FIXME: add a FIXIT to remove the trailing comma.
+ Diag(Tok, diag::err_extraneous_trailing_comma);
} else if (LHS.isInvalid()) {
for (auto &E : ArgExprs)
Actions.CorrectDelayedTyposInExpr(E);
diff --git a/clang/test/Parser/recovery.cpp b/clang/test/Parser/recovery.cpp
index 4e2811c4cac926..e318461e1961da 100644
--- a/clang/test/Parser/recovery.cpp
+++ b/clang/test/Parser/recovery.cpp
@@ -215,3 +215,10 @@ struct ::template foo, struct ::template bar; // expected-error 2 {{expected ide
struct ::foo struct::; // expected-error {{no struct named 'foo' in the global namespace}} expected-error {{expected identifier}}
class :: : {} a; // expected-error {{expected identifier}} expected-error {{expected class name}}
}
+
+namespace GH125225 {
+void func(int);
+void k() {
+ func(1, ); // expected-error {{extraneous trailing comma}}
+}
+}
|
clang/lib/Parse/ParseExpr.cpp
Outdated
| @@ -2237,6 +2237,9 @@ Parser::ParsePostfixExpressionSuffix(ExprResult LHS) { | |||
| if (PP.isCodeCompletionReached() && !CalledSignatureHelp) | |||
| RunSignatureHelp(); | |||
| LHS = ExprError(); | |||
| } else if (!HasError && HasTrailingComma) { | |||
| // FIXME: add a FIXIT to remove the trailing comma. | |||
| Diag(Tok, diag::err_extraneous_trailing_comma); | |||
There was a problem hiding this comment.
I think we should just reuse err_expected_expression. The important point here is that there is a diagnostic.
Not sure this happens often enough to warrant a fixit
There was a problem hiding this comment.
Reused the existing err_expected_expression.
I initially thought that the new err_extraneous_trailing_comma provided a clearer message than err_expected_expression, but it only applies to a specific test case (the one added in this PR).
For other cases, such as function2Arg(1, );, the new diagnostic is incorrect.
|
Can you add a release note and a more detailed description? Thanks |
Done. I think we don't need a release note, this is a regression fix, and #114684 is not released yet. |
hokein
force-pushed
the
fix-trailing-comma
branch
from
7df0f5b to
8f8693a
Compare
|
@hokein feel free to merge :) |
Thanks for the ping. We have several instances of Additionally, we will need to backport this fix to the |
|
/cherry-pick 922f339 |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/16226 Here is the relevant piece of the build log for the reference |
|
/pull-request #127215 |
) This patch fixes a regression caused by llvm#114684 where clang accepts trailing commas for function calls. Fixes llvm#125225
) This patch fixes a regression caused by llvm#114684 where clang accepts trailing commas for function calls. Fixes llvm#125225 (cherry picked from commit 922f339)
|
This seems to break something with variadic args and the gcc hack with ## to ignore the trailing comma error: At least with See godbolt test here: https://godbolt.org/z/fejP1oa1f But given that there is a difference between the foo and bar tests maybe I've missed some special rule for when this variadic macro solution with ## applies. |
|
clang19 rejects this code as well, https://godbolt.org/z/Yf96W8KWv. To make this case work, I think we should use the
|
|
Thanks. I see now that it did not work with older clang (at least not 19.1.0). It did however work with our downstream fork before this patch. I haven't figured out how we avoided the error in the past. I guess we must have something downstream that avoided the error in the past, but I can't remember that we've done anything actively to suppress such errors. Weird. I also agree that our downstream users should try to use VA_OPT instead (hopefully there are no other tools that would complain about that). |
) This patch fixes a regression caused by llvm#114684 where clang accepts trailing commas for function calls. Fixes llvm#125225
This patch fixes a regression caused by #114684 where clang accepts trailing commas for function calls.
Fixes #125225