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

Joel E. Denny via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 16 20:07:19 PDT 2019

jdenny added a comment.

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

> In D59712#1469358 <https://reviews.llvm.org/D59712#1469358>, @jdenny wrote:
> > 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.)
> >
> Does this pass `check-all`? `check-all` of stage-2? test-suite?

No.  The assert breaks cases in at least ExprConstant.cpp and SemaExpr.cpp.

However, the current version of the patch does not break check-all.  I have not tried the others.

Moreover, I see now that this patch fixes behavior for the following:

  #include <climits>
  #include <iostream>
  constexpr unsigned foo() {
    unsigned i = INT_MAX;
    ++i; // should not warn value is outside range
    return i;
  int main() {
    std::cout << foo() << '\n';
    std::cout << (1 << (unsigned)-1) << '\n'; // should not warn about neg shift count
    return 0;

I'll integrate these into the tests later.



More information about the cfe-commits mailing list