Messages in this thread | | | Subject | Re: [PATCH] platform/surface: aggregator: shut up clang -Wconstantn-conversion warning | From | Hans de Goede <> | Date | Fri, 14 May 2021 21:38:52 +0200 |
| |
Hi,
On 5/14/21 4:21 PM, Maximilian Luz wrote: > On 5/14/21 4:05 PM, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> Clang complains about the assignment of SSAM_ANY_IID to >> ssam_device_uid->instance: >> >> drivers/platform/surface/surface_aggregator_registry.c:478:25: error: implicit conversion from 'int' to '__u8' (aka 'unsigned char') changes value from 65535 to 255 [-Werror,-Wconstant-conversion] >> { SSAM_VDEV(HUB, 0x02, SSAM_ANY_IID, 0x00) }, >> ~ ^~~~~~~~~~~~ >> include/linux/surface_aggregator/device.h:71:23: note: expanded from macro 'SSAM_ANY_IID' >> #define SSAM_ANY_IID 0xffff >> ^~~~~~ >> include/linux/surface_aggregator/device.h:126:63: note: expanded from macro 'SSAM_VDEV' >> SSAM_DEVICE(SSAM_DOMAIN_VIRTUAL, SSAM_VIRTUAL_TC_##cat, tid, iid, fun) >> ^~~ >> include/linux/surface_aggregator/device.h:102:41: note: expanded from macro 'SSAM_DEVICE' >> .instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0, \ >> ^~~ >> >> The assignment doesn't actually happen, but clang checks the type limits >> before checking whether this assignment is reached. Shut up the warning >> using an explicit type cast. > > I'm not too happy about this fix as (I believe) it will also shut up any > valid GCC error message in case those macros are used with non-u8 (and > non-SSAM_ANY_xxx) values.
Since you're the maintainer of this code, I'll go with your judgement here, esp. since as the commit msg states SSAM_ANY_IID is never actually assigned to .instance, instead it gets set to 0.
So this is a false-positive compiler warning, which may be best fixed in the compiler itself.
With that said I think this is the second time this comes up now, maybe we should add a comment to the code about the clang warning ?
Regards,
Hans
> > I'll let others judge and decide what's preferred, however. > > In case you're deciding to apply this, feel free to add: > > Reviewed-by: Maximilian Luz <luzmaximilian@gmail.com> > > Thanks, > Max > >> Fixes: eb0e90a82098 ("platform/surface: aggregator: Add dedicated bus and device type") >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> --- >> include/linux/surface_aggregator/device.h | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/include/linux/surface_aggregator/device.h b/include/linux/surface_aggregator/device.h >> index 4441ad667c3f..90df092ed565 100644 >> --- a/include/linux/surface_aggregator/device.h >> +++ b/include/linux/surface_aggregator/device.h >> @@ -98,9 +98,9 @@ struct ssam_device_uid { >> | (((fun) != SSAM_ANY_FUN) ? SSAM_MATCH_FUNCTION : 0), \ >> .domain = d, \ >> .category = cat, \ >> - .target = ((tid) != SSAM_ANY_TID) ? (tid) : 0, \ >> - .instance = ((iid) != SSAM_ANY_IID) ? (iid) : 0, \ >> - .function = ((fun) != SSAM_ANY_FUN) ? (fun) : 0 \ >> + .target = ((tid) != SSAM_ANY_TID) ? (u8)(tid) : 0, \ >> + .instance = ((iid) != SSAM_ANY_IID) ? (u8)(iid) : 0, \ >> + .function = ((fun) != SSAM_ANY_FUN) ? (u8)(fun) : 0 \ >> /** >> * SSAM_VDEV() - Initialize a &struct ssam_device_id as virtual device with >> >
| |