[PATCH] D119609: [Clang][Sema] Prohibit expr statement in the default argument

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D119609#3426279 <https://reviews.llvm.org/D119609#3426279>, @erichkeane wrote:

> I think LGTM on technical perspective, Please don't commit until @aaron.ballman confirms he is OK with the direction, or would like to wait longer for GCC.

I spotted some technical issues with the diagnostic wording. As for waiting for GCC, my concern there is largely with the fact that people use statement expressions in macros so that they can define local variables that don't impact the outer scope where the macro is expanded. I could see such a macro being used as a default argument, so I worry a bit about breaking code in that case. However, C doesn't have default arguments, so this isn't an issue there. And C++ has lambdas which can be called in a default argument, so users have some ability to fix their code in the event of an error so long as they're in C++11 or later. Because of that, I would be okay moving forward with this, but cautiously in case someone reports code that breaks and can't easily be fixed (or significant breakage in an important 3rd party header).



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4383-4384
   "%select{default|copy|move}0 constructor">;
+def err_expr_statement_in_default_arg : Error<
+  "expression statement not permitted in default argument">;
 
----------------
I get mixed up every time, but I double-checked -- these are statement expressions, not expression statements. I also think the diagnostic should be more clear about default argument vs default non-type template argument.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:7073
+              Actions.ActOnParamDefaultArgumentError(Param, EqualLoc);
+              // Skip the expression statement and continue parsing
+              SkipUntil(tok::comma, StopBeforeMatch);
----------------



================
Comment at: clang/test/Sema/err-expr-stmt-in-default-arg.cpp:1
+// RUN: %clang_cc1 %s -fsyntax-only -verify -std=c++20
+
----------------
You should rename this test to `stmt-expr-in-default-arg.cpp`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119609