[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

[Joel E. Denny via Phabricator via llvm-commits]

jdenny added a comment.

In D59712#1469301 <https://reviews.llvm.org/D59712#1469301>, @lebedev.ri wrote:

> In D59712#1469295 <https://reviews.llvm.org/D59712#1469295>, @craig.topper wrote:
>
> > Wondering if it would be better to assert for asking for the sign of an unsigned APSInt. I could see a caller just wanting to get the msb for some reason and not knowing that isNegative won’t work.
>
>
> Yes, i, too, would think an assert is much better solution (since i literally just tripped over this in this review.)


Imagine a caller wants to check for a negative value for an APSInt from an arbitrary expression.  Whether the value is negative is important, but not why.  Given that APSInt is documented as knowing its signedness, it seems unintuitive that the caller is responsible for first checking for an unsigned type to avoid an internal compiler error.

If the fear is that developers are too used to calling isNegative to check the high bit, maybe isNegative should always (regardless of signedness) fail an assert for APSInt, and we should find different function names that won't cause such confusion.

Whatever we do for isNegative, we should probably do something similar for isNonNegative as it has the same issues.


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

https://reviews.llvm.org/D59712