[PATCH] D44559: [Sema] Wrong width of result of mul operation

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D44559#1044653, @aaron.ballman wrote:

> In https://reviews.llvm.org/D44559#1044639, @rjmccall wrote:
>
> > In https://reviews.llvm.org/D44559#1044186, @avt77 wrote:
> >
> > > >> In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote:
> > > >> 
> > > >>> I think we're correct not to warn here and that GCC/ICC are being noisy.  The existence of a temporary promotion to a wider type doesn't justify warning on arithmetic between two operands that are the same size as the ultimate result.  It is totally fair for users to think of this operation as being "closed" on the original type.
> > > >> 
> > > >> 
> > > >> Could you please clarify, are you saying that PR35409 <https://bugs.llvm.org/show_bug.cgi?id=35409> is not a bug, and clang should continue to not warn in those cases?
> > > > 
> > > > Correct.
> > >
> > > Does it mean we should abandon this revision? On the other hand it's a real bug, isn't it?
> >
> >
> > Not as I see it, no.
>
>
> Do you see this code as having a bug when `a` is >= 182?
>
>   short foo(unsigned char a) {
>     return a * a;
>   }
>
>
> (If you don't like seeing `unsigned char`  you can imagine it was spelled as `uint8_t`.)


That's an interesting question.  In general, these warnings do try to ignore the effects of implicit promotion.  We would not want -Wsign-conversion to fire on `unsigned short x = an_unsigned_short + 1;` (or `- 1`, for that matter), even though formally this coerces a `signed int` to `unsigned short`.  Similarly, -Wsign-conversion *should* warn on `signed short x = an_unsigned_short + 1;`, even though formally the promotion from `unsigned short` to `signed int` is not problematic and the final conversion from `signed int` to `signed short` is not a signedness change.  (This second example should also generate a -Wconversion warning, but the questions are independent.)  Applying that strictly here would say that the user is entitled to think of this as an operation on `unsigned char` that then gets losslessly promoted to `signed short`, even though arithmetically that's not what happens.  On the other hand, I do think there's some room for -Wsign-conversion to be more aggressive than -Wconversion about this sort of thing; -Wsign-conversion should generally fire for any changes in signedness from the original operand types (with the usual complexities around constant values), and there's just an exception for computations whose value is known to fit within the expressible range of the result type, which is not true of this multiplication.  So I think it would be acceptable to warn on this.

John.


https://reviews.llvm.org/D44559