{"thread":{"id":"45906","subject":"[PATCH v2] archive-tar: fix a sparse 'constant too large' warning","startedAt":"2017-05-08T20:35:05Z","lastAt":"2017-05-10T10:55:43Z","messageCount":6,"participants":["Ramsay Jones","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"319115","messageId":"4bf9be3c-5d19-09e3-b8b3-dd58e00e623d@ramsayjones.plus.com","threadId":"45906","inReplyTo":null,"subject":"[PATCH v2] archive-tar: fix a sparse 'constant too large' warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-05-08T20:34:58Z","receivedAt":"2017-05-08T20:35:05Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nCommit dddbad728c (\"timestamp_t: a new data type for timestamps\",\n26-04-2017) introduced a new typedef 'timestamp_t', as a synonym for an\nunsigned long, which was used at the time to represent timestamps in\ngit. A later commit 28f4aee3fb (\"use uintmax_t for timestamps\",\n26-04-2017) changed the typedef to use an 'uintmax_t' for the timestamp\nrepresentation type.\n\nWhen building on a 32-bit Linux system, sparse complains that a constant\n(USTAR_MAX_MTIME) used to detect a 'far-future mtime' timestamp, is too\nlarge; 'warning: constant 077777777777UL is so big it is unsigned long\nlong' on lines 335 and 338 of archive-tar.c. Note that both gcc and\nclang only issue a warning if this constant is used in a context that\nrequires an 'unsigned long' (rather than an uintmax_t). (Since TIME_MAX\nis no longer equal to 0xFFFFFFFF, even on a 32-bit system, the macro\nUSTAR_MAX_MTIME is set to 077777777777UL, which cannot be represented as\nan 'unsigned long' constant).\n\nIn order to suppress the warning, change the definition of the macro\nconstant USTAR_MAX_MTIME to use an 'ULL' type suffix.\n\nIn a similar vein, on systems which use a 64-bit representation of the\n'unsigned long' type, the USTAR_MAX_SIZE constant macro is defined with\nthe value 077777777777ULL. Although this does not cause any warning\nmessages to be issued, it would be more appropriate for this constant\nto use an 'UL' type suffix rather than 'ULL'.\n\nSigned-off-by: Ramsay Jones \n---\n\nHi Junio,\n\nSorry for being a bit slow with this, but this is the v2 version\nof the patch that I promised, which includes the change to the\nUSTAR_MAX_SIZE macro that Johannes requested.\n\nThanks.\n\nATB,\nRamsay Jones\n\n archive-tar.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex 319a5b1c7..073e60ebd 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -28,12 +28,12 @@ static int write_tar_filter_archive(const struct archiver *ar,\n #if ULONG_MAX == 0xFFFFFFFF\n #define USTAR_MAX_SIZE ULONG_MAX\n #else\n-#define USTAR_MAX_SIZE 077777777777ULL\n+#define USTAR_MAX_SIZE 077777777777UL\n #endif\n #if TIME_MAX == 0xFFFFFFFF\n #define USTAR_MAX_MTIME TIME_MAX\n #else\n-#define USTAR_MAX_MTIME 077777777777UL\n+#define USTAR_MAX_MTIME 077777777777ULL\n #endif\n \n /* writes out the whole block, but only if it is full */\n-- \n2.12.0\n"},{"id":"319177","messageId":"alpine.DEB.2.21.1.1705091223190.146734@virtualbox","threadId":"45906","inReplyTo":"4bf9be3c-5d19-09e3-b8b3-dd58e00e623d@ramsayjones.plus.com","subject":"Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-09T10:24:31Z","receivedAt":"2017-05-09T10:24:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ramsay,\n\nOn Mon, 8 May 2017, Ramsay Jones wrote:\n\n> Commit dddbad728c (\"timestamp_t: a new data type for timestamps\",\n> 26-04-2017) introduced a new typedef 'timestamp_t', as a synonym for an\n> unsigned long, which was used at the time to represent timestamps in\n> git. A later commit 28f4aee3fb (\"use uintmax_t for timestamps\",\n> 26-04-2017) changed the typedef to use an 'uintmax_t' for the timestamp\n> representation type.\n> \n> When building on a 32-bit Linux system, sparse complains that a constant\n> (USTAR_MAX_MTIME) used to detect a 'far-future mtime' timestamp, is too\n> large; 'warning: constant 077777777777UL is so big it is unsigned long\n> long' on lines 335 and 338 of archive-tar.c. Note that both gcc and\n> clang only issue a warning if this constant is used in a context that\n> requires an 'unsigned long' (rather than an uintmax_t). (Since TIME_MAX\n> is no longer equal to 0xFFFFFFFF, even on a 32-bit system, the macro\n> USTAR_MAX_MTIME is set to 077777777777UL, which cannot be represented as\n> an 'unsigned long' constant).\n> \n> In order to suppress the warning, change the definition of the macro\n> constant USTAR_MAX_MTIME to use an 'ULL' type suffix.\n> \n> In a similar vein, on systems which use a 64-bit representation of the\n> 'unsigned long' type, the USTAR_MAX_SIZE constant macro is defined with\n> the value 077777777777ULL. Although this does not cause any warning\n> messages to be issued, it would be more appropriate for this constant\n> to use an 'UL' type suffix rather than 'ULL'.\n\n\tThe reason for the current situation is that an earlier fix for\n\tthe USTAR_MAX_MTIME constant was applied to the USTAR_MAX_SIZE\n\tconstant by mistake.\n\n> Signed-off-by: Ramsay Jones \n\nWith that addition to the commit message: ACK\n\nCiao,\nDscho\n"},{"id":"319223","messageId":"5068a9b6-c77a-737d-9d80-40108c5f22c3@ramsayjones.plus.com","threadId":"45906","inReplyTo":"alpine.DEB.2.21.1.1705091223190.146734@virtualbox","subject":"Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-05-09T20:13:01Z","receivedAt":"2017-05-09T20:13:10Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 09/05/17 11:24, Johannes Schindelin wrote:\n> Hi Ramsay,\n> \n> On Mon, 8 May 2017, Ramsay Jones wrote:\n> \n>> Commit dddbad728c (\"timestamp_t: a new data type for timestamps\",\n>> 26-04-2017) introduced a new typedef 'timestamp_t', as a synonym for an\n>> unsigned long, which was used at the time to represent timestamps in\n>> git. A later commit 28f4aee3fb (\"use uintmax_t for timestamps\",\n>> 26-04-2017) changed the typedef to use an 'uintmax_t' for the timestamp\n>> representation type.\n>>\n>> When building on a 32-bit Linux system, sparse complains that a constant\n>> (USTAR_MAX_MTIME) used to detect a 'far-future mtime' timestamp, is too\n>> large; 'warning: constant 077777777777UL is so big it is unsigned long\n>> long' on lines 335 and 338 of archive-tar.c. Note that both gcc and\n>> clang only issue a warning if this constant is used in a context that\n>> requires an 'unsigned long' (rather than an uintmax_t). (Since TIME_MAX\n>> is no longer equal to 0xFFFFFFFF, even on a 32-bit system, the macro\n>> USTAR_MAX_MTIME is set to 077777777777UL, which cannot be represented as\n>> an 'unsigned long' constant).\n>>\n>> In order to suppress the warning, change the definition of the macro\n>> constant USTAR_MAX_MTIME to use an 'ULL' type suffix.\n>>\n>> In a similar vein, on systems which use a 64-bit representation of the\n>> 'unsigned long' type, the USTAR_MAX_SIZE constant macro is defined with\n>> the value 077777777777ULL. Although this does not cause any warning\n>> messages to be issued, it would be more appropriate for this constant\n>> to use an 'UL' type suffix rather than 'ULL'.\n> \n> \tThe reason for the current situation is that an earlier fix for\n> \tthe USTAR_MAX_MTIME constant was applied to the USTAR_MAX_SIZE\n> \tconstant by mistake.\n\nYeah, I had a similar comment in the commit message (but much more\nverbose than your concise addition above), but I edited it several\ntimes, without finding a wording that I liked. I eventually removed\nit, because it didn't really add any value. :(\n\n>> Signed-off-by: Ramsay Jones \n> \n> With that addition to the commit message: ACK\n\nThis patch is now in the 'next' branch, so I guess it's too late\nto add this to the commit message (which I would be quite happy to do).\n\nWell, at the beginning of the next cycle, 'next' will be rebuilt, so\nI guess (if we remember!) this patch could be updated then.\n\nATB,\nRamsay Jones\n\n"},{"id":"319238","messageId":"xmqqvap9zjpc.fsf@gitster.mtv.corp.google.com","threadId":"45906","inReplyTo":"4bf9be3c-5d19-09e3-b8b3-dd58e00e623d@ramsayjones.plus.com","subject":"Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-09T23:36:31Z","receivedAt":"2017-05-09T23:36:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> In a similar vein, on systems which use a 64-bit representation of the\n> 'unsigned long' type, the USTAR_MAX_SIZE constant macro is defined with\n> the value 077777777777ULL. Although this does not cause any warning\n> messages to be issued, it would be more appropriate for this constant\n> to use an 'UL' type suffix rather than 'ULL'.\n\n... it is more appropriate because we know the recipient is\n\"unsigned long\", not \"unsigned long long\", in this case? As opposed\nto the case of timestamp_t, which is opaque and could be \"unsigned\nlong long\"?\n\nThat makes sense to me, even though it took a bit of thinking aloud\nto understand.\n\nLooks good; thanks.\n"},{"id":"319239","messageId":"xmqqr2zxzjf9.fsf@gitster.mtv.corp.google.com","threadId":"45906","inReplyTo":"5068a9b6-c77a-737d-9d80-40108c5f22c3@ramsayjones.plus.com","subject":"Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-09T23:42:34Z","receivedAt":"2017-05-09T23:42:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> Yeah, I had a similar comment in the commit message (but much more\n> verbose than your concise addition above), but I edited it several\n> times, without finding a wording that I liked. I eventually removed\n> it, because it didn't really add any value. :(\n\nI tend to agree that the proposed additional comment does not add\nmuch value. It assures the readers that we (at the time of applying\nthis patch) know that the earlier use of ULL was not done with a\ngood reason but was merely an accident, and strengthens the claim\nthat this is a good change, but the correctness of the change is\nalready obvious, and the readers would understand without being\nexplained where the incorrectness we have to fix with this patch\ncame from, I would think.\n.\n"},{"id":"319270","messageId":"alpine.DEB.2.21.1.1705101254440.146734@virtualbox","threadId":"45906","inReplyTo":"xmqqr2zxzjf9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-05-10T10:55:25Z","receivedAt":"2017-05-10T10:55:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 10 May 2017, Junio C Hamano wrote:\n\n> Ramsay Jones writes:\n> \n> > Yeah, I had a similar comment in the commit message (but much more\n> > verbose than your concise addition above), but I edited it several\n> > times, without finding a wording that I liked. I eventually removed\n> > it, because it didn't really add any value. :(\n> \n> I tend to agree that the proposed additional comment does not add much\n> value. It assures the readers that we (at the time of applying this\n> patch) know that the earlier use of ULL was not done with a good reason\n> but was merely an accident, and strengthens the claim that this is a\n> good change, but the correctness of the change is already obvious, and\n> the readers would understand without being explained where the\n> incorrectness we have to fix with this patch came from, I would think.\n\nFuture me would find that comment in the commit message very clarifying,\nthough: why was that code there? Ah, that's why.\n\nNow I have to dig through the mailing list to find out.\n\nCiao,\nDscho\n"}]}