[PATCH] D81420: Fix size for _ExtInt types with builtins

[Mott, Jeffrey T via Phabricator via cfe-commits]

jtmott-intel marked 6 inline comments as done.
jtmott-intel added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7939
   "to a non-const integer (%0 invalid)">;
+def err_overflow_builtin_extint_size : Error<
+  "_ExtInt argument larger than 64-bits to overflow builtin requires runtime "
----------------
rjmccall wrote:
> jtmott-intel wrote:
> > rjmccall wrote:
> > > erichkeane wrote:
> > > > Mentioning target-specific support here seems incorrect. @rjmccall I cannot think of a better wording, can you?
> > > "overflow builtins do not support _ExtInt operands of more than %0 bits on this target"?  I don't think it's unreasonable to mention the target-specificness of it.   Hard-coding the number 64 in the diagnostic seems excessively targeted, though.
> > I discovered there's a good existing message I could use that already takes the bitwidth as a parameter. I decided not to add a new one. Thoughts/preferences? Here's how the message would come out.
> > 
> >     test.c:5:43: error: signed _ExtInt of bit sizes greater than 128 not supported
> >         _Bool status = __builtin_mul_overflow(x, y, &result);
> >                                               ^
> >     1 error generated.
> > 
> It'd be much clearer to say something about the fact that it's just this builtin that's not supported.
Updated message to something halfway between John and Erich's suggestions. Current result:

    test.c:5:43: error: __builtin_mul_overflow does not support signed _ExtInt operands of more than 128 bits
        _Bool status = __builtin_mul_overflow(x, y, &result);
                                              ^
    1 error generated.




================
Comment at: clang/test/Sema/builtins-overflow.c:39
+    _ExtInt(129) result;
+    _Bool status = __builtin_mul_overflow(x, y, &result); // expected-error {{signed _ExtInt of bit sizes greater than 128 not supported}}
+  }
----------------
erichkeane wrote:
> As @rjmccall said, I'd suggest the new message anyway that mentions the builtin, otherwise this would be confusing for me.  Something like:
> 
> signed argument to __builtin_mul_overflow must have a bitwidth of less than or equal to 128.
Updated message to something halfway between your and John's suggestions. Current result:

    test.c:5:43: error: __builtin_mul_overflow does not support signed _ExtInt operands of more than 128 bits
        _Bool status = __builtin_mul_overflow(x, y, &result);
                                              ^
    1 error generated.




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81420/new/

https://reviews.llvm.org/D81420