> -----Original Message----- > From: Jacek Kowalski <[email protected]> > Sent: Tuesday, July 8, 2025 11:35 AM > To: Loktionov, Aleksandr <[email protected]>; Nguyen, > Anthony L <[email protected]>; Kitszel, Przemyslaw > <[email protected]>; Andrew Lunn <[email protected]>; > David S. Miller <[email protected]>; Eric Dumazet > <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni > <[email protected]>; Simon Horman <[email protected]> > Cc: [email protected]; [email protected] > Subject: Re: [Intel-wired-lan] [PATCH iwl-next v2 5/5] ixgbe: drop > unnecessary constant casts to u16 > > >> - checksum = (u16)IXGBE_EEPROM_SUM - checksum; > >> + checksum = IXGBE_EEPROM_SUM - checksum; > >> > > Can't lead to different results, especially when: > > checksum > IXGBE_EEPROM_SUM → the result becomes negative in int, > and narrowing to u16 causes unexpected wraparound? > > > > With this patch you are changing the semantics of the code - from > explicit 16bit arithmetic to full int implicit promotion which can be > error-prone or compiler-dependent /* for different targets */. > > > As far as I understand the C language does this by design - in the > terms of C specification: > > > If an int can represent all values of the original type (...), the > value is converted to an int; otherwise, it is converted to an > unsigned int. These are called the integer promotions. (see note) > > > > (note) The integer promotions are applied only: as part of the usual > arithmetic conversions, (...) > > > And subtraction semantics are: > > > If both operands have arithmetic type, the usual arithmetic > conversions are performed on them. > > > > So there is no *16 bit arithmetic* - it is always done on integers > (usually 32 bits). > > Or have I missed something? > > > Additionally I've checked AMD64, ARM and MIPS assembly output from GCC > and clang on https://godbolt.org/z/GPsMxrWfe - both of the following > snippets compile to exactly the same assembly: > > #define NVM_SUM 0xBABA > int test(int num) { > volatile unsigned short test = 0xFFFF; > unsigned short checksum = NVM_SUM - test; > return checksum; > } > > vs.: > > #define NVM_SUM 0xBABA > int test(int num) { > volatile unsigned short test = 0xFFFF; > unsigned short checksum = ((unsigned short)NVM_SUM) - test; > return checksum; > } > > -- > Best regards, > Jacek Kowalski
Thank you for the tests, I've copy pasted into Godbolt and' verified a doesn't of targets, the code is identical got gcc. So, the change looks scary for the first glance, but GCC actually handles it the same way. Reviewed-by: Aleksandr Loktionov <[email protected]>
