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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59712/new/ https://reviews.llvm.org/D59712 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
