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

[John McCall via Phabricator via llvm-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D44559#1049049, @avt77 wrote:

> > If that operation overflows, so be it — we're not going to warn about the potential for overflow every time the user adds two ints, and by the same token, it doesn't make any sense to warn about every time the user adds two shorts just because the language made this otherwise-unimportant technical decision to do the arithmetic in a wider type.
>
> There is one comment only: this patch changes the 'mul' operation only and initially the change was done for X86 only because it clearly says about "doubling" of mul result width.


Yes, I understand that addition was not changed by the patch.  I do not think that using different logic for multiplication is appropriate just because multiplication produces bigger values.  It is reasonable to multiply two 8-bit numbers and store the result in an 8-bit variable without a warning because the net effect is just doing a "closed" multiplication on an 8-bit type, which is a reasonable operation to want to do.  GCC is too noisy here.

There is nothing x86-specific about the supposed problem here, and using target-specific logic was always clearly inappropriate.

> And another comment: currently we have warning here:
> 
>   int c = a * b;
>   T d1 = c; // expected-warning{{implicit conversion loses integer precision: 'int' to 'char'}}
>    
> 
> but we don't have it here:
> 
> T d2 = a * b; // when T is char
> 
> It's a potential problem from my point of view.

All of these warnings are single-line analyses, which is unlikely to change absent massive architectural improvements to clang.  It has nothing to do with there being a multiplication; we would give you that exact same warning if you wrote `int c = a;`.

In https://reviews.llvm.org/D44559#1049057, @lebedev.ri wrote:

> In https://reviews.llvm.org/D44559#1049049, @avt77 wrote:
>
> > ...
> >  But I don't mind to close the review - and the corresponding bug of course.
>
>
> I disagree with closing the bug.
>  It's a real issue that is not detected/diagnosed by anything in clang (diagnostic, sanitizer).
>
> It seems we are in a deadlock here :)


I don't have any special authority here besides having designed and implemented the current diagnostic.  You are welcome to start a thread on cfe-dev to gather support for changing the design of -Wconversion to always warn about the potential for overflow in sub-int multiplication. I will argue against you there, like I have here, but if there's consensus that we should warn about this, I'll abide by the community's judgment.


https://reviews.llvm.org/D44559