Re: [PATCH] c: don't drop typedef information in casts

[Martin Sebor <msebor@gmail.com>]

On 3/9/21 6:46 PM, David Lamparter wrote:
> On Wed, Mar 10, 2021 at 02:28:15AM +0100, David Lamparter wrote:
>> The TYPE_MAIN_VARIANT() here was, for casts to a typedef'd type name,
>> resulting in all information about the typedef's involvement getting
>> lost.  This drops necessary information for warnings and can make them
>> confusing or even misleading.  It also makes specialized warnings for
>> unspecified-size system types (pid_t, uid_t, ...) impossible.
> 
> I hope I didn't produce any glaring SNAFUs in submitting this patch, I
> haven't previously contributed to GCC.
> 
> I've run the testsuite on x86_64 and seen no breakage from this.  The
> delta (compared to this run:
> https://gcc.gnu.org/pipermail/gcc-testresults/2021-March/662078.html) is:

Thanks for the patch and for clarifying how you tested the change!

> 
> $ diff -U0 /tmp/before.sum /tmp/after.sum
> --- /tmp/before.sum     2021-03-09 22:48:26.989146223 +0100
> +++ /tmp/after.sum      2021-03-10 01:46:46.209935875 +0100
> @@ -89 +89,2 @@
> -FAIL: g++.old-deja/g++.other/virtual2.C  -std=c++2a execution test
> +FAIL: g++.dg/modules/xtreme-header-5_c.C -std=c++17 (internal compiler error)
> +FAIL: g++.dg/modules/xtreme-header-5_c.C -std=c++17 (test for excess errors)
> 
> None of which seems to be related to the patch.  (The 2 new fails are
> "xtreme-header-5_c.C:3:30: internal compiler error: same canonical type node for different types 'void' and 'std::__void_t<typename _Func::is_transparent>'")

Yes, these are unrelated C++ failures.

A few comments on the patch.  It took me a bit at first to see
the problem it tries to solve.  I see the improvement but I'm not
sure it's quite correct, or what in the test verifies the desired
outcome.  The warning on line 12 before the change is:

cast-5.c:13:7: warning: conversion from ‘int’ to ‘unsigned char’ may 
change value [-Wconversion]

and after:

cast-5.c:13:7: warning: conversion from ‘qualtypedefname’ {aka ‘int’} to 
‘unsigned char’ may change value [-Wconversion]

There's no volatile, so the test passes either way.  More important,
though, qualtypedefname is a typedef for volatile int but the AKA
type says it's int.  So the new output doesn't seem right.

In any case, the test should fail without the patch.  Using
dg-warning to look for both the expected typedef name and the AKA
type would seem like a better to do that than dg-bogus.

As an aside, although it's not a requirement, it's helpful to open
a bug first, before submitting a patch.  It's more overhead but it
gives the problem more visibility, lets us track regressions and
related bugs, and help decide what fixes should be backported.  In
this case, the same improvement can also be made to the C++ front
end and having a bug might help highlight that.

Finally, GCC is in a regression-fixing stage now, so unless this
problem is one (having a bug report would also help determine that),
this patch will likely need to wait until general development
reopens sometime in May.  In any case, it's a maintainer's call
to approve it.

Thanks again!

Martin