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

[David Lamparter <equinox@diac24.net>]

On Wed, Mar 10, 2021 at 10:01:39AM -0700, Martin Sebor wrote:
> 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.

> 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.

I've tried making it a bit more clear in a bug opened as requested:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99526

> 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.

The new output is actually correct since casts produce rvalues.  I was
trying to put together a test for that to make sure the volatile is NOT
there (in an earlier version it was, and that actually broke things.)

That said, I seem to have b0rked the test anyway since on closer look it
says: "ERROR: gcc.dg/cast-5.c: unknown dg option: */ for */"  (I had
missed that since I looked for PASS/FAIL...) - so need to fix that
anyway.

> 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.

Thanks for the input, I need to look a bit more at how this whole stuff
works.  Going to poke it a bit more...

Also, in seeing your response re. the volatile, it occured to me that
stripping qualifiers on a typedef and still calling it the typedef is
unhelpful.  Ideally, my goal is:

typedef int i;
typedef const int ci;
typeof((i)x) = i
typeof((ci)x) = int  (<-- w/ my patch = ci; this is confusing)
typeof((const i)x) = i

(Without my patch, all 3 typeofs come out as plain int.)

So I think I need to somehow check whether the qualifiers are part of
the typedef or not;  essentially peel typedefs until it's one without
qualifiers.

> As an aside, although it's not a requirement, it's helpful to open
> a bug first, before submitting a patch. [...]

Linked above :)

> 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.

Ah, I hadn't seen that.  My bad.


Thanks for the input!

-David