r180603 - C++1y: support simple variable assignments in constexpr functions.

[David Blaikie]

On Mon, May 20, 2013 at 9:33 AM, Hans Wennborg <hans at chromium.org> wrote:

> On Mon, May 20, 2013 at 5:14 PM, Jordan Rose <jordan_rose at apple.com>
> wrote:
> >
> > On May 20, 2013, at 6:59 , Hans Wennborg <hans at chromium.org> wrote:
> >
> >> Hi Richard,
> >>
> >> On Fri, Apr 26, 2013 at 3:36 PM, Richard Smith
> >> <richard-llvm at metafoo.co.uk> wrote:
> >>> Author: rsmith
> >>> Date: Fri Apr 26 09:36:30 2013
> >>> New Revision: 180603
> >>>
> >>> URL: http://llvm.org/viewvc/llvm-project?rev=180603&view=rev
> >>> Log:
> >>> C++1y: support simple variable assignments in constexpr functions.
> >>
> >> This seems to have caused -Winteger-overflow to become much more
> aggressive.
> >>
> >> I don't know what in your code causes this, but I bisected it down to
> >> this revision.
> >>
> >> The following code didn't use to warn, but now does (only in C++11 mode
> though):
> >>
> >> #include <time.h>
> >> #include <limits>
> >>
> >> void f() {
> >>  long long x;
> >>  x = std::numeric_limits<time_t>::min() * 1000;
> >> }
> >>
> >> /tmp/a.cc:6:42: warning: overflow in expression; result is 0 with type
> >> 'long' [-Winteger-overflow]
> >>  x = std::numeric_limits<time_t>::min() * 1000;
> >>                                         ^
> >>
> >> This is causing problems in Chromium where we have such code, but we
> >> know we won't hit it on platforms where time_t is 64-bit.
> >>
> >> Was the warning behaviour change intentional?
> >
> > That looks like a real warning to me:
> > - if time_t is signed 32-bit and int is 32-bit, the min value will be
> -2^31, and multiplying by 1000 will result in negative overflow
> > - if time_t is signed 64-bit, the min value will be -2^63, and
> multiplying by 1000 will result in negative overflow
> >
> > The correct way to handle the 32-bit case would be to use "1000LL", so
> that the calculation is done in long-long space (64 bits) instead of
> int-space (presumably 32 bits).
>
> Sorry, my example was a simplified version of the original Chromium
> code. We do handle the 32-bit case, 1000 is really a 64-bit constant
> variable (static const long long kMillisecondsPerSecond = 1000).
>
> > The 64-bit case will never be right unless your long long is 128 bits.
>
> Right, but we never hit that code in the 64-bit case. The problem is
> that Clang doesn't know that. And it doesn't help if I add very
> explicit guards either, I can do this:
>
> void f() {
>   long long x;
>   if (sizeof(time_t) == 4)
>     x = std::numeric_limits<time_t>::min() * 1000LL;
>   else
>     x = 5;
> }
>
> and it will still warn.
>
> > In general any improvements to constexpr will up the aggressiveness of
> warnings based on expression values, since the compiler can calculate more
> values at compile time. I think this is generally a good thing.
>
> Yes, I'm just wondering if this effect was intended here before I
> start jumping through hoops to avoid it.


Chances are this warning just needs to be reclassified/moved to an
AnalysisBasedWarning so that it doesn't fire on possibly-unreachable code.
Your example above is a fairly classic one of how people might reasonable
write this & we use similar solutions for other warnings with such issues.

(alternatively, you can rewrite your checks with preprocessor macros
instead - but then you loose the syntax checking on that code when you're
in the other build mode, of course, which is unfortunate/undesirable)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130520/32c25f0f/attachment.html>