Jason Merrill <ja...@redhat.com> writes: > On 8/28/24 3:59 PM, Arsen Arsenović wrote: >> Hm, maybe-unused-1.C should be moved into the previous patch. Could do >> before pushing. > > Let's combine these three into a single patch, they're all small and closely > related.
Sure, that's fine by me.
>> @@ -3147,7 +3147,13 @@ maybe_promote_temps (tree *stmt, void *d)
>> to run the initializer.
>> If the initializer is a conditional expression, we need to collect
>> and declare any promoted variables nested within it. DTORs for such
>> - variables must be run conditionally too. */
>> + variables must be run conditionally too.
>> +
>> + Since here we're synthetically processing code here, we've already
>> + emitted any Wunused-result warnings.
>
> If it's already been through convert_to_void for the warnings, why isn't it
> already void?
The conversions making it void can get lost during the flattening and
transformation of statements containing CO_AWAIT_EXPRs (or, more
accurately, the CO_AWAIT_EXPRs get isolated without the conversion
making it void, it'd seem).
>>>> This patch fixes a gcc-15 regression (PR116502) and an inelegance in my
>>>> earlier patch related to converting CO_AWAIT_EXPRs to void.
>>>> This wasn't noticed anywhere AFAICT, but it was a bit unfortunate.
>>>> The other two patches fix an actual regression as well as provide
>>>> handling for converting INDIRECT_REFs wrapping CO_AWAIT_EXPRs (which I
>>>> noticed was lacking while triaging the regression).
>>>> WRT INDIRECT_REFs, I've considered making convert_to_void recurse on the
>>>> INDIRECT_REF case, so that special handling doesn't need to be added
>>>> when some expression might end up getting wrapped in a INDIRECT_REF. Is
>>>> there a reason why we don't do that?
>>>
>>> Primarily because that isn't what the language says. And for instance if I
>>> have
>>>
>>> int *volatile p;
>>> *p;
>>>
>>> I think just recursing will wrongly warn about not accessing the value of p.
>> Hmm, with the change in the OP I didn't get that warning, but that
>> reason is compelling enough for me.
>
> On second thought, I think we should split the INDIRECT_REF handling between
> REFERENCE_REF_P and actual * expressions, and recurse for warnings in the
> reference case (but don't return the value of the recursive call). And drop
> the warn_nodiscard from the * case; we shouldn't ever get a nodiscard warning
> about *f();.
This seems reasonable to me, but by not returning the value of the
recursive call, we might get double diagnostics. Consider, for
instance: void h(int* x) { static_cast<int&>(*x); }
Here, stripping away the INDIRECT_REF leads and calling convert_to_void
on the NOP_EXPR results in a "statement has no effect" warning. If we
then continue with executing the outer convert_to_void, we get that
diagnostic again (assuming you intended the convert_to_void to be
discarded). I might be misunderstanding you, but I started with the
patch below:
modified gcc/cp/cvt.cc
@@ -1403,7 +1403,15 @@ convert_to_void (tree expr, impl_conv_void implicit,
tsubst_flags_t complain)
gcc_unreachable ();
}
}
- if (is_reference || !is_volatile || !is_complete || TREE_ADDRESSABLE
(type))
+
+ if (is_reference)
+ {
+ expr = TREE_OPERAND (expr, 0);
+ convert_to_void (expr, implicit, complain);
+ break;
+ }
+
+ if (!is_volatile || !is_complete || TREE_ADDRESSABLE (type))
{
/* Emit a warning (if enabled) when the "effect-less" INDIRECT_REF
operation is stripped off. Note that we don't warn about
@@ -1417,10 +1428,7 @@ convert_to_void (tree expr, impl_conv_void implicit,
tsubst_flags_t complain)
&& !warning_suppressed_p (expr, OPT_Wunused_value)
&& !is_reference)
warning_at (loc, OPT_Wunused_value, "value computed is not
used");
expr = TREE_OPERAND (expr, 0);
- if (TREE_CODE (expr) == CALL_EXPR
- && (complain & tf_warning))
- maybe_warn_nodiscard (expr, implicit);
}
break;
... this results in a double diagnostic. Altering the is_reference case
to be:
if (is_reference)
{
expr = convert_to_void (TREE_OPERAND (expr, 0), implicit, complain);
break;
}
... prevents the double diagnostic, but in that case, we'd probably not
get any more work later as EXPR would already be void, so it seems
redundant.
--
Arsen Arsenović
signature.asc
Description: PGP signature
