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

This revision was automatically updated to reflect the committed changes. Closed by commit rC359012: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types (authored by jdenny, committed by ). Changed prior to commit: https://reviews.llvm.org/D59712?vs=195626=196274#toc Repository: rC

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

jdenny added a comment. Thanks for the reviews! Will push soon. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59712/new/ https://reviews.llvm.org/D59712 ___ cfe-commits mailing list

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

lebedev.ri added a comment. The `APSInt.h` itself looks good to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59712/new/ https://reviews.llvm.org/D59712 ___ cfe-commits mailing list

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

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. In D59712#1473223 , @jdenny wrote: > In D59712#1469392 , @lebedev.ri > wrote: > > > Does this pass

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

jdenny added a comment. In D59712#1469392 , @lebedev.ri wrote: > Does this pass `check-all`? `check-all` of stage-2? test-suite? For me, all these tests behave with the current patch. As before, the only subproject I did not build was llgo.

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

hfinkel added a comment. In D59712#1472544 , @jdenny wrote: > In D59712#1472358 , @hfinkel wrote: > > > > I've never tried running the other tests you mention, for any patch. I > > > thought people normally left

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

jdenny added a comment. In D59712#1472358 , @hfinkel wrote: > > 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? > > We have a lot

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

hfinkel added a comment. In D59712#1472318 , @jdenny wrote: > In D59712#1469760 , @lebedev.ri > wrote: > > > In D59712#1469693 , @jdenny wrote: > > > > > In D59712#1469392

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

jdenny added a comment. In D59712#1472342 , @lebedev.ri wrote: > In D59712#1472318 , @jdenny wrote: > > > For me, check-all's success is not affected by the current patch. I built > > all subprojects except llgo,

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

lebedev.ri resigned from this revision. lebedev.ri added a comment. In D59712#1472318 , @jdenny wrote: > In D59712#1469760 , @lebedev.ri > wrote: > > > In D59712#1469693 ,

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

jdenny added a comment. In D59712#1469760 , @lebedev.ri wrote: > In D59712#1469693 , @jdenny wrote: > > > In D59712#1469392 , @lebedev.ri > > wrote: > > > > > In

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

lebedev.ri added a comment. In D59712#1469693 , @jdenny wrote: > In D59712#1469392 , @lebedev.ri > wrote: > > > In D59712#1469358 , @jdenny wrote: > > > > > In

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

jdenny added a comment. In D59712#1469392 , @lebedev.ri wrote: > In D59712#1469358 , @jdenny wrote: > > > In D59712#1469301 , @lebedev.ri > > wrote: > > > > > In

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

lebedev.ri added a comment. In D59712#1469358 , @jdenny wrote: > In D59712#1469301 , @lebedev.ri > wrote: > > > In D59712#1469295 , @craig.topper > > wrote: > > > > >

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

jdenny added a comment. In D59712#1469301 , @lebedev.ri wrote: > In D59712#1469295 , @craig.topper > wrote: > > > Wondering if it would be better to assert for asking for the sign of an > > unsigned APSInt. I

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

lebedev.ri added a comment. In 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

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

jdenny updated this revision to Diff 195452. jdenny added a comment. Duplicate new APSInt test but for signed type. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59712/new/ https://reviews.llvm.org/D59712 Files: clang/test/OpenMP/distribute_collapse_messages.cpp

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

craig.topper added a comment. 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59712/new/

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

lebedev.ri marked an inline comment as done. lebedev.ri added inline comments. Comment at: llvm/unittests/ADT/APSIntTest.cpp:162 +TEST(APSIntTest, UnsignedHighBit) { + APSInt False(APInt(1, 0)); Can you please duplicate this whole test but for signed

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

jdenny marked an inline comment as done. jdenny added inline comments. Comment at: llvm/unittests/ADT/APSIntTest.cpp:188-194 + EXPECT_FALSE(CharBoundaryUnder.isNegative()); + EXPECT_TRUE(CharBoundaryUnder.isNonNegative()); +

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

lebedev.ri requested changes to this revision. lebedev.ri added inline comments. This revision now requires changes to proceed. Comment at: llvm/unittests/ADT/APSIntTest.cpp:188-194 + EXPECT_FALSE(CharBoundaryUnder.isNegative()); +

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

jdenny added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59712/new/ https://reviews.llvm.org/D59712 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

jdenny added a comment. Thanks. Then the patch just needs someone to review from the ADT side. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59712/new/ https://reviews.llvm.org/D59712 ___ cfe-commits

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

ABataev added a comment. I'm fine with the changes in the OpenMP tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59712/new/ https://reviews.llvm.org/D59712 ___ cfe-commits mailing list

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

jdenny added a comment. Ping. To be clear, my goal here was not to change the OpenMP behavior. My goal was to fix APSInt behavior. If people feel that the old OpenMP behavior is better somehow, we should surely find another way to implement that. Repository: rG LLVM Github Monorepo

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

jdenny updated this revision to Diff 191938. jdenny added a comment. Extend llvm/unittests/ADT/APSIntTest.cpp. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59712/new/ https://reviews.llvm.org/D59712 Files:

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

jdenny added a comment. In D59712#1439966 , @lebedev.ri wrote: > Might warrant test coverage in `llvm/unittest/ADT/APSIntTest.cpp`. Thanks. I'll work on that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

lebedev.ri added a comment. Might warrant test coverage in `llvm/unittest/ADT/APSIntTest.cpp`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59712/new/ https://reviews.llvm.org/D59712 ___ cfe-commits

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

jdenny created this revision. jdenny added reviewers: ABataev, chandlerc, craig.topper. Herald added subscribers: jdoerfert, dexonsmith, guansong. Herald added a project: LLVM. Without this patch, APSInt inherits APInt::isNegative, which merely checks the sign bit without regard to whether the