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
