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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.

In D59712#1472318 <https://reviews.llvm.org/D59712#1472318>, @jdenny wrote:

> In D59712#1469760 <https://reviews.llvm.org/D59712#1469760>, @lebedev.ri wrote:
>
> > In D59712#1469693 <https://reviews.llvm.org/D59712#1469693>, @jdenny wrote:
> >
> > > 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.
> >
> >
> > Err, i was talking about the current code in the patch :)
>
>
> For me, check-all's success is not affected by the current patch.  I built all subprojects except llgo, which gave me a build problem independently of this patch.


(Built natively, or with clang with this patch?)

> I've never tried running the other tests you mention, for any patch.  I thought people normally left those to the bots.  Should this patch be handled differently?

Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59712