latest gcc vs lib/timespec.h:85

[Jim Meyering]

Hi Paul,
Mainly just a heads up, since this certainly isn't blocking me.

When trying to build coreutils using gcc built from very recent (with
some change committed since Sep 26), I see this new warning/error:

In file included from src/system.h:140:0,
from src/ls.c:84:
src/ls.c: In function 'print_long_format':
./lib/timespec.h:85:11: error: assuming signed overflow does not occur
when simplifying conditional to constant [-Werror=strict-overflow]
return (a.tv_sec < b.tv_sec ? -1
~~~~~~~~~~~~~~~~~~~~~~~~~
: a.tv_sec > b.tv_sec ? 1
^~~~~~~~~~~~~~~~~~~~~~~~~
: (int) (a.tv_nsec - b.tv_nsec));
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

When compiling with gcc built from latest sources on September 26,
there is no such problem.

Given all of the comments on that function, I'd be tempted to suppress
this warning in that function.

[Paul Eggert]

Post by Jim Meyering
Given all of the comments on that function, I'd be tempted to suppress
this warning in that function.

That would work. Another possibility would be to include verify.h and
add something like this to the start of timespec_cmp:

  assume (-1 <= a.tv_nsec && a.tv_nsec <= 2 * TIMESPEC_RESOLUTION);

  assume (-1 <= b.tv_nsec && b.tv_nsec <= 2 * TIMESPEC_RESOLUTION);

We might be able to make these 'assume' calls fancier, to exactly match
the comments, but I'm not sure it's worth the bother.

[Jim Meyering]

Post by Jim Meyering
Given all of the comments on that function, I'd be tempted to suppress
this warning in that function.

That would work. Another possibility would be to include verify.h and add
assume (-1 <= a.tv_nsec && a.tv_nsec <= 2 * TIMESPEC_RESOLUTION);
assume (-1 <= b.tv_nsec && b.tv_nsec <= 2 * TIMESPEC_RESOLUTION);
We might be able to make these 'assume' calls fancier, to exactly match the
comments, but I'm not sure it's worth the bother.

Thanks. I prefer that. Here's a proposed patch:

[Jim Meyering]

Post by Jim Meyering
Given all of the comments on that function, I'd be tempted to suppress
this warning in that function.

That would work. Another possibility would be to include verify.h and add
assume (-1 <= a.tv_nsec && a.tv_nsec <= 2 * TIMESPEC_RESOLUTION);
assume (-1 <= b.tv_nsec && b.tv_nsec <= 2 * TIMESPEC_RESOLUTION);
We might be able to make these 'assume' calls fancier, to exactly match the
comments, but I'm not sure it's worth the bother.

Pushed.

[Paul Eggert]

I prefer 'assume' to 'assure' here, since it's a low-level time-comparison
primitive and lots of other code in the module already silently assumes that the
timestamps are valid. Also, while I was in the neighborhood I noticed that the
cast is no longer needed, since the module provokes -Wconversion warnings in
several other places now (and I expect nobody notices because nobody looks at
those warnings any more). So I installed the attached followup.

[Jim Meyering]

Post by Paul Eggert
I prefer 'assume' to 'assure' here, since it's a low-level time-comparison
primitive and lots of other code in the module already silently assumes that
the timestamps are valid. Also, while I was in the neighborhood I noticed
that the cast is no longer needed, since the module provokes -Wconversion
warnings in several other places now (and I expect nobody notices because
nobody looks at those warnings any more). So I installed the attached
followup.

Oh, yes. Definitely prefer assume. Thanks for the fix.

[Tim Rühsen]

Post by Jim Meyering

Post by Paul Eggert
I prefer 'assume' to 'assure' here, since it's a low-level time-comparison
primitive and lots of other code in the module already silently assumes that
the timestamps are valid. Also, while I was in the neighborhood I noticed
that the cast is no longer needed, since the module provokes -Wconversion
warnings in several other places now (and I expect nobody notices because
nobody looks at those warnings any more). So I installed the attached
followup.

Oh, yes. Definitely prefer assume. Thanks for the fix.

Now clang throws out an annoying warning about the return value of
timespec_cmp():

In file included from wget.c:51:
../lib/timespec.h:94:20: warning: implicit conversion loses integer
precision: 'long' to 'int' [-Wshorten-64-to-32]
return a.tv_nsec - b.tv_nsec;
~~~~~~ ~~~~~~~~~~^~~~~~~~~~~

I wonder if we can't silence clang and gcc by keeping the 'assume()'
*and* using return (int) (a.tv_nsec - b.tv_nsec));

WDYT ?

With Best Regards, Tim

[Paul Eggert]

Now clang throws out an annoying warning about the return value of > timespec_cmp(): > > In file included from wget.c:51: >

../lib/timespec.h:94:20: warning: implicit conversion loses integer >
precision: 'long' to 'int' [-Wshorten-64-to-32] > return a.tv_nsec -
b.tv_nsec; > ~~~~~~ ~~~~~~~~~~^~~~~~~~~~~ > > I wonder if we can't
silence clang and gcc by keeping the 'assume()' > *and* using return
(int) (a.tv_nsec - b.tv_nsec));
I'd rather continue to omit the cast, as casts are too powerful (it's
too easy to get them wrong, with no diagnostic).

-Wshorten-64-to-32 is like -Wconversion, and we should ask people not to
use -Wshorten-64-to-32 in the same way that we ask them not to use
-Wconversion. Does it fix things to add -Wshorten-64-to-32 to
build-aux/gcc-warning.spec and to build-aux/g++-warning.spec?

[Tim Rühsen]

Post by Paul Eggert

Now clang throws out an annoying warning about the return value of  >
timespec_cmp(): > > In file included from wget.c:51: >

../lib/timespec.h:94:20: warning: implicit conversion loses integer >
precision: 'long' to 'int' [-Wshorten-64-to-32] > return a.tv_nsec -
b.tv_nsec; > ~~~~~~ ~~~~~~~~~~^~~~~~~~~~~ > > I wonder if we can't
silence clang and gcc by keeping the 'assume()' > *and* using return
(int) (a.tv_nsec - b.tv_nsec));
I'd rather continue to omit the cast, as casts are too powerful (it's
too easy to get them wrong, with no diagnostic).
-Wshorten-64-to-32 is like -Wconversion, and we should ask people not to
use -Wshorten-64-to-32 in the same way that we ask them not to use
-Wconversion. Does it fix things to add -Wshorten-64-to-32 to
build-aux/gcc-warning.spec and to build-aux/g++-warning.spec?

No, it doesn't change anything (I am not using manywarnings.m4).

What about a #pragma here ?

And of course I can disable the warning for the gnulib code alone...
that isn't the point.
Switching on warnings for the gnulib code was meant as a help in the
means "more eyes see more things". It was once was a developers who
asked for not switching off warnings in gnulib code and I agreed. But I
don't definitely don't want to waste both your and my time with nitpicking.

With Best Regards, Tim

[Paul Eggert]

Does it fix things to add -Wshorten-64-to-32 to build-aux/gcc-warning.spec and to build-aux/g++-warning.spec? > No, it doesn't change anything (I am not using manywarnings.m4).

OK, in that case you should be able to fix the problem by specifying
-Wno-shorten-64-to-32 in addition to whatever other flags that you are
specifying by hand.