Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning

[Johannes Schindelin]

Hi Junio,

On Wed, 10 May 2017, Junio C Hamano wrote:

> Ramsay Jones  writes:
> 
> > Yeah, I had a similar comment in the commit message (but much more
> > verbose than your concise addition above), but I edited it several
> > times, without finding a wording that I liked. I eventually removed
> > it, because it didn't really add any value. :(
> 
> I tend to agree that the proposed additional comment does not add much
> value.  It assures the readers that we (at the time of applying this
> patch) know that the earlier use of ULL was not done with a good reason
> but was merely an accident, and strengthens the claim that this is a
> good change, but the correctness of the change is already obvious, and
> the readers would understand without being explained where the
> incorrectness we have to fix with this patch came from, I would think.

Future me would find that comment in the commit message very clarifying,
though: why was that code there? Ah, that's why.

Now I have to dig through the mailing list to find out.

Ciao,
Dscho

Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning

[Junio C Hamano]

Ramsay Jones  writes:

> Yeah, I had a similar comment in the commit message (but much more
> verbose than your concise addition above), but I edited it several
> times, without finding a wording that I liked. I eventually removed
> it, because it didn't really add any value. :(

I tend to agree that the proposed additional comment does not add
much value.  It assures the readers that we (at the time of applying
this patch) know that the earlier use of ULL was not done with a
good reason but was merely an accident, and strengthens the claim
that this is a good change, but the correctness of the change is
already obvious, and the readers would understand without being
explained where the incorrectness we have to fix with this patch
came from, I would think.
.

Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning

[Junio C Hamano]

Ramsay Jones  writes:

> In a similar vein, on systems which use a 64-bit representation of the
> 'unsigned long' type, the USTAR_MAX_SIZE constant macro is defined with
> the value 0777ULL. Although this does not cause any warning
> messages to be issued, it would be more appropriate for this constant
> to use an 'UL' type suffix rather than 'ULL'.

... it is more appropriate because we know the recipient is
"unsigned long", not "unsigned long long", in this case?  As opposed
to the case of timestamp_t, which is opaque and could be "unsigned
long long"?

That makes sense to me, even though it took a bit of thinking aloud
to understand.

Looks good; thanks.

Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning

[Ramsay Jones]

On 09/05/17 11:24, Johannes Schindelin wrote:
> Hi Ramsay,
> 
> On Mon, 8 May 2017, Ramsay Jones wrote:
> 
>> Commit dddbad728c ("timestamp_t: a new data type for timestamps",
>> 26-04-2017) introduced a new typedef 'timestamp_t', as a synonym for an
>> unsigned long, which was used at the time to represent timestamps in
>> git. A later commit 28f4aee3fb ("use uintmax_t for timestamps",
>> 26-04-2017) changed the typedef to use an 'uintmax_t' for the timestamp
>> representation type.
>>
>> When building on a 32-bit Linux system, sparse complains that a constant
>> (USTAR_MAX_MTIME) used to detect a 'far-future mtime' timestamp, is too
>> large; 'warning: constant 0777UL is so big it is unsigned long
>> long' on lines 335 and 338 of archive-tar.c. Note that both gcc and
>> clang only issue a warning if this constant is used in a context that
>> requires an 'unsigned long' (rather than an uintmax_t). (Since TIME_MAX
>> is no longer equal to 0x, even on a 32-bit system, the macro
>> USTAR_MAX_MTIME is set to 0777UL, which cannot be represented as
>> an 'unsigned long' constant).
>>
>> In order to suppress the warning, change the definition of the macro
>> constant USTAR_MAX_MTIME to use an 'ULL' type suffix.
>>
>> In a similar vein, on systems which use a 64-bit representation of the
>> 'unsigned long' type, the USTAR_MAX_SIZE constant macro is defined with
>> the value 0777ULL. Although this does not cause any warning
>> messages to be issued, it would be more appropriate for this constant
>> to use an 'UL' type suffix rather than 'ULL'.
> 
>   The reason for the current situation is that an earlier fix for
>   the USTAR_MAX_MTIME constant was applied to the USTAR_MAX_SIZE
>   constant by mistake.

Yeah, I had a similar comment in the commit message (but much more
verbose than your concise addition above), but I edited it several
times, without finding a wording that I liked. I eventually removed
it, because it didn't really add any value. :(

>> Signed-off-by: Ramsay Jones 
> 
> With that addition to the commit message: ACK

This patch is now in the 'next' branch, so I guess it's too late
to add this to the commit message (which I would be quite happy to do).

Well, at the beginning of the next cycle, 'next' will be rebuilt, so
I guess (if we remember!) this patch could be updated then.

ATB,
Ramsay Jones

Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning

[Johannes Schindelin]

Hi Ramsay,

On Mon, 8 May 2017, Ramsay Jones wrote:

> Commit dddbad728c ("timestamp_t: a new data type for timestamps",
> 26-04-2017) introduced a new typedef 'timestamp_t', as a synonym for an
> unsigned long, which was used at the time to represent timestamps in
> git. A later commit 28f4aee3fb ("use uintmax_t for timestamps",
> 26-04-2017) changed the typedef to use an 'uintmax_t' for the timestamp
> representation type.
> 
> When building on a 32-bit Linux system, sparse complains that a constant
> (USTAR_MAX_MTIME) used to detect a 'far-future mtime' timestamp, is too
> large; 'warning: constant 0777UL is so big it is unsigned long
> long' on lines 335 and 338 of archive-tar.c. Note that both gcc and
> clang only issue a warning if this constant is used in a context that
> requires an 'unsigned long' (rather than an uintmax_t). (Since TIME_MAX
> is no longer equal to 0x, even on a 32-bit system, the macro
> USTAR_MAX_MTIME is set to 0777UL, which cannot be represented as
> an 'unsigned long' constant).
> 
> In order to suppress the warning, change the definition of the macro
> constant USTAR_MAX_MTIME to use an 'ULL' type suffix.
> 
> In a similar vein, on systems which use a 64-bit representation of the
> 'unsigned long' type, the USTAR_MAX_SIZE constant macro is defined with
> the value 0777ULL. Although this does not cause any warning
> messages to be issued, it would be more appropriate for this constant
> to use an 'UL' type suffix rather than 'ULL'.

The reason for the current situation is that an earlier fix for
the USTAR_MAX_MTIME constant was applied to the USTAR_MAX_SIZE
constant by mistake.

> Signed-off-by: Ramsay Jones 

With that addition to the commit message: ACK

Ciao,
Dscho

[PATCH v2] archive-tar: fix a sparse 'constant too large' warning

[Ramsay Jones]

Commit dddbad728c ("timestamp_t: a new data type for timestamps",
26-04-2017) introduced a new typedef 'timestamp_t', as a synonym for an
unsigned long, which was used at the time to represent timestamps in
git. A later commit 28f4aee3fb ("use uintmax_t for timestamps",
26-04-2017) changed the typedef to use an 'uintmax_t' for the timestamp
representation type.

When building on a 32-bit Linux system, sparse complains that a constant
(USTAR_MAX_MTIME) used to detect a 'far-future mtime' timestamp, is too
large; 'warning: constant 0777UL is so big it is unsigned long
long' on lines 335 and 338 of archive-tar.c. Note that both gcc and
clang only issue a warning if this constant is used in a context that
requires an 'unsigned long' (rather than an uintmax_t). (Since TIME_MAX
is no longer equal to 0x, even on a 32-bit system, the macro
USTAR_MAX_MTIME is set to 0777UL, which cannot be represented as
an 'unsigned long' constant).

In order to suppress the warning, change the definition of the macro
constant USTAR_MAX_MTIME to use an 'ULL' type suffix.

In a similar vein, on systems which use a 64-bit representation of the
'unsigned long' type, the USTAR_MAX_SIZE constant macro is defined with
the value 0777ULL. Although this does not cause any warning
messages to be issued, it would be more appropriate for this constant
to use an 'UL' type suffix rather than 'ULL'.

Signed-off-by: Ramsay Jones 
---

Hi Junio,

Sorry for being a bit slow with this, but this is the v2 version
of the patch that I promised, which includes the change to the
USTAR_MAX_SIZE macro that Johannes requested.

Thanks.

ATB,
Ramsay Jones

 archive-tar.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/archive-tar.c b/archive-tar.c
index 319a5b1c7..073e60ebd 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -28,12 +28,12 @@ static int write_tar_filter_archive(const struct archiver 
*ar,
 #if ULONG_MAX == 0x
 #define USTAR_MAX_SIZE ULONG_MAX
 #else
-#define USTAR_MAX_SIZE 0777ULL
+#define USTAR_MAX_SIZE 0777UL
 #endif
 #if TIME_MAX == 0x
 #define USTAR_MAX_MTIME TIME_MAX
 #else
-#define USTAR_MAX_MTIME 0777UL
+#define USTAR_MAX_MTIME 0777ULL
 #endif
 
 /* writes out the whole block, but only if it is full */
-- 
2.12.0