On Thu, Feb 04, 2021 at 03:47:54PM -0500, Jason Merrill via Gcc-patches wrote:
> On 2/4/21 1:11 PM, Marek Polacek wrote:
> > On Thu, Feb 04, 2021 at 10:59:21AM -0500, Jason Merrill wrote:
> > > On 2/3/21 7:03 PM, Marek Polacek wrote:
> > > > Since most of volatile is deprecated in C++20, we are required to warn
> > > > for compound assignments to volatile variables and so on. But here we
> > > > have
> > > >
> > > > volatile int x, y, z;
> > > > (b ? x : y) = 1;
> > > >
> > > > and we shouldn't warn, because simple assignments like x = 24; should
> > > > not provoke the warning when they are a discarded-value expression.
> > > >
> > > > We warn here because when ?: is used as an lvalue, we transform it in
> > > > cp_build_modify_expr/COND_EXPR from (a ? b : c) = rhs to
> > > >
> > > > (a ? (b = rhs) : (c = rhs))
> > > >
> > > > and build_conditional_expr then calls mark_lvalue_use for the new
> > > > artificial assignments
> > >
> > > Hmm, that seems wrong; the ?: expression itself does not use lvalue
> > > operands
> > > any more than ',' does. I notice that removing those mark_lvalue_use
> > > calls
> > > doesn't regress Wunused-var-10.c, which was added with them in r160289.
> >
> > The mark_lvalue_use calls didn't strike me as wrong because [expr.cond]/7
> > says that lvalue-to-rvalue conversion is performed on the second and third
> > operands.
>
> Only after we've decided (in /6) that the result is a prvalue.
Oh, I see. :~
> > With those mark_lvalue_use calls removed, we'd not issue the
> > warning for
> >
> > (b ? (x = 2) : y) = 1;
> > (b ? x : (y = 5)) = 1;
>
> Why wouldn't we? The assignment should call mark_lvalue_use for the LHS,
> which recursively applies it to the arms of the ?:.
Aha: we call mark_lvalue_use_*nonread* and the warning was guarded by
read_p. I don't know why I did it, I see no regressions if I just
remove the check. And then I get the expected results.
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/10?
-- >8 --
Since most of volatile is deprecated in C++20, we are required to warn
for compound assignments to volatile variables and so on. But here we
have
volatile int x, y, z;
(b ? x : y) = 1;
and we shouldn't warn, because simple assignments like x = 24; should
not provoke the warning when they are a discarded-value expression.
We warn here because when ?: is used as an lvalue, we transform it in
cp_build_modify_expr/COND_EXPR from (a ? b : c) = rhs to
(a ? (b = rhs) : (c = rhs))
and build_conditional_expr then calls mark_lvalue_use for the new
artificial assignments, which then evokes the warning. The calls
to mark_lvalue_use were added in r160289 to suppress warnings in
Wunused-var-10.c, but looks like they're no longer needed.
To warn on
(b ? (x = 2) : y) = 1;
(b ? x : (y = 5)) = 1;
I've tweaked a check in mark_use/MODIFY_EXPR.
I'd argue this is a regression because GCC 9 doesn't warn.
gcc/cp/ChangeLog:
PR c++/98947
* call.c (build_conditional_expr_1): Don't call mark_lvalue_use
on arg2/arg3.
* expr.c (mark_use) <case MODIFY_EXPR>: Don't check read_p when
issuing the -Wvolatile warning. Only set TREE_THIS_VOLATILE if
a warning was emitted.
gcc/testsuite/ChangeLog:
PR c++/98947
* g++.dg/cpp2a/volatile5.C: New test.
---
gcc/cp/call.c | 2 --
gcc/cp/expr.c | 14 +++++++-------
gcc/testsuite/g++.dg/cpp2a/volatile5.C | 15 +++++++++++++++
3 files changed, 22 insertions(+), 9 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp2a/volatile5.C
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index c7e13f3a22b..4744c9768ec 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -5559,8 +5559,6 @@ build_conditional_expr_1 (const op_location_t &loc,
&& same_type_p (arg2_type, arg3_type))
{
result_type = arg2_type;
- arg2 = mark_lvalue_use (arg2);
- arg3 = mark_lvalue_use (arg3);
goto valid_operands;
}
diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c
index 480e740f08c..d16d1896f2d 100644
--- a/gcc/cp/expr.c
+++ b/gcc/cp/expr.c
@@ -224,17 +224,17 @@ mark_use (tree expr, bool rvalue_p, bool read_p,
a volatile-qualified type is deprecated unless the assignment
is either a discarded-value expression or appears in an
unevaluated context." */
- if (read_p
- && !cp_unevaluated_operand
+ if (!cp_unevaluated_operand
&& (TREE_THIS_VOLATILE (lhs)
|| CP_TYPE_VOLATILE_P (TREE_TYPE (lhs)))
&& !TREE_THIS_VOLATILE (expr))
{
- warning_at (location_of (expr), OPT_Wvolatile,
- "using value of simple assignment with %<volatile%>-"
- "qualified left operand is deprecated");
- /* Make sure not to warn about this assignment again. */
- TREE_THIS_VOLATILE (expr) = true;
+ if (warning_at (location_of (expr), OPT_Wvolatile,
+ "using value of simple assignment with "
+ "%<volatile%>-qualified left operand is "
+ "deprecated"))
+ /* Make sure not to warn about this assignment again. */
+ TREE_THIS_VOLATILE (expr) = true;
}
break;
}
diff --git a/gcc/testsuite/g++.dg/cpp2a/volatile5.C
b/gcc/testsuite/g++.dg/cpp2a/volatile5.C
new file mode 100644
index 00000000000..1f9d23845b4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp2a/volatile5.C
@@ -0,0 +1,15 @@
+// PR c++/98947
+// { dg-do compile }
+
+volatile int x, y, z;
+
+void
+f (bool b)
+{
+ (b ? x : y) = 1;
+ (b ? x : y) += 1; // { dg-warning "compound assignment" "" { target c++20 } }
+ z = (b ? x : y) = 1; // { dg-warning "using value of simple assignment" "" {
target c++20 } }
+ ((z = 2) ? x : y) = 1; // { dg-warning "using value of simple assignment" ""
{ target c++20 } }
+ (b ? (x = 2) : y) = 1; // { dg-warning "using value of simple assignment" ""
{ target c++20 } }
+ (b ? x : (y = 5)) = 1; // { dg-warning "using value of simple assignment" ""
{ target c++20 } }
+}
base-commit: 4e7c24d97dd65083a770252ce942f43d408fe11d
--
2.29.2
