{"thread":{"id":"29492","subject":"[PATCH] vcs-svn: Fix some compiler warnings","startedAt":"2012-01-31T18:48:47Z","lastAt":"2012-02-02T22:18:58Z","messageCount":16,"participants":["Ramsay Jones","Jonathan Nieder","Junio C Hamano","Dmitry Ivankov","David Barr"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"183424","messageId":"4F28378F.6080108@ramsay1.demon.co.uk","threadId":"29492","inReplyTo":null,"subject":"[PATCH] vcs-svn: Fix some compiler warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-01-31T18:48:47Z","receivedAt":"2012-01-31T18:48:47Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nIn particular, some versions of gcc complains as follows:\n\n        CC vcs-svn/sliding_window.o\n    vcs-svn/sliding_window.c: In function `check_overflow':\n    vcs-svn/sliding_window.c:36: warning: comparison is always false \\\n        due to limited range of data type\n\n        CC vcs-svn/fast_export.o\n    vcs-svn/fast_export.c: In function `fast_export_blob_delta':\n    vcs-svn/fast_export.c:303: warning: comparison is always false due \\\n        to limited range of data type\n\nSimply casting the (limited range unsigned) variable in the comparison\nto an uintmax_t does not suppress the warning, however, since gcc is\n\"smart\" enough to know that the cast does not change anything regarding\nthe value of the casted expression. In order to suppress the warning, we\nreplace the variable with a new uintmax_t variable initialized with the\nvalue of the original variable.\n\nNote that the \"some versions of gcc\" which complain includes 3.4.4 and\n4.1.2, whereas gcc version 4.4.0 compiles the code without complaint.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n\nHi Jonathan,\n\nWhen I wrote this patch, your \"jn/svn-fe\" branch was still in pu, so I was\ngoing to ask you to squash this, or some variant, into any re-roll ...\n\nNote, in the original version of this patch, the change to check_overflow()\nwas much simpler; it was effectively the same 2-line change (modulo some\nvariable names) which is applied to fast_export_blob_delta(). But it just\ndidn't look right (not sure why!), which prompted the renaming of the\nfunction parameter names. This is, obviously, an unrelated change, so maybe\nthe change to sliding_window.c should be reverted to the 2-line change ...\n\nATB,\nRamsay Jones\n\n vcs-svn/fast_export.c    |    3 ++-\n vcs-svn/sliding_window.c |   11 ++++++-----\n 2 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c\nindex 19d7c34..6edd37e 100644\n--- a/vcs-svn/fast_export.c\n+++ b/vcs-svn/fast_export.c\n@@ -300,7 +300,8 @@ void fast_export_blob_delta(uint32_t mode,\n \t\t\t\tuint32_t len, struct line_buffer *input)\n {\n \tlong postimage_len;\n-\tif (len > maximum_signed_value_of_type(off_t))\n+\tuintmax_t delta_len = (uintmax_t) len;\n+\tif (delta_len > maximum_signed_value_of_type(off_t))\n \t\tdie(\"enormous delta\");\n \tpostimage_len = apply_delta((off_t) len, input, old_data, old_mode);\n \tif (mode == REPO_MODE_LNK) {\ndiff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c\nindex 1bac7a4..49a7293 100644\n--- a/vcs-svn/sliding_window.c\n+++ b/vcs-svn/sliding_window.c\n@@ -31,15 +31,16 @@ static int read_to_fill_or_whine(struct line_buffer *file,\n \treturn 0;\n }\n \n-static int check_overflow(off_t a, size_t b)\n+static int check_overflow(off_t offset, size_t len)\n {\n-\tif (b > maximum_signed_value_of_type(off_t))\n+\tuintmax_t delta_len = (uintmax_t) len;\n+\tif (delta_len > maximum_signed_value_of_type(off_t))\n \t\treturn error(\"unrepresentable length in delta: \"\n-\t\t\t\t\"%\"PRIuMAX\" > OFF_MAX\", (uintmax_t) b);\n-\tif (signed_add_overflows(a, (off_t) b))\n+\t\t\t\t\"%\"PRIuMAX\" > OFF_MAX\", delta_len);\n+\tif (signed_add_overflows(offset, (off_t) len))\n \t\treturn error(\"unrepresentable offset in delta: \"\n \t\t\t\t\"%\"PRIuMAX\" + %\"PRIuMAX\" > OFF_MAX\",\n-\t\t\t\t(uintmax_t) a, (uintmax_t) b);\n+\t\t\t\t(uintmax_t) offset, delta_len);\n \treturn 0;\n }\n \n-- \n1.7.9\n"},{"id":"183425","messageId":"20120131192053.GC12443@burratino","threadId":"29492","inReplyTo":"4F28378F.6080108@ramsay1.demon.co.uk","subject":"Re: [PATCH] vcs-svn: Fix some compiler warnings","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-01-31T19:20:53Z","receivedAt":"2012-01-31T19:20:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nRamsay Jones wrote:\n\n> In particular, some versions of gcc complains as follows:\n> \n>         CC vcs-svn/sliding_window.o\n>     vcs-svn/sliding_window.c: In function `check_overflow':\n>     vcs-svn/sliding_window.c:36: warning: comparison is always false \\\n>         due to limited range of data type\n\nYuck.  Suppressing this warning would presumably also suppress the\noptimization that notices the comparison is always false.\n\nThe -Wtype-limits warning also triggers in some other perfectly\nreasonable situations: see <http://gcc.gnu.org/PR51712>.  I wonder if\nwe should keep a list of unreliable warnings somewhere (e.g.,\nMeta/Make).\n\n[...]\n> Note that the \"some versions of gcc\" which complain includes 3.4.4 and\n> 4.1.2, whereas gcc version 4.4.0 compiles the code without complaint.\n\nThanks for tracking this down.  Interesting.  -Wtype-limits was split\nout from the default set of warnings (!) in gcc 4.3 to address\n<http://gcc.gnu.org/PR12963>, among other bugs (r124875, 2007-05-20).\n\n[...]\n> --- a/vcs-svn/fast_export.c\n> +++ b/vcs-svn/fast_export.c\n> @@ -300,7 +300,8 @@ void fast_export_blob_delta(uint32_t mode,\n>  \t\t\t\tuint32_t len, struct line_buffer *input)\n>  {\n>  \tlong postimage_len;\n> -\tif (len > maximum_signed_value_of_type(off_t))\n> +\tuintmax_t delta_len = (uintmax_t) len;\n> +\tif (delta_len > maximum_signed_value_of_type(off_t))\n>  \t\tdie(\"enormous delta\");\n>  \tpostimage_len = apply_delta((off_t) len, input, old_data, old_mode);\n\nIs there some less ugly way to write the condition \"if this value is\nnot representable in this type\"?\n\nI guess I could live with something like the following (please don't\ntake the names too seriously):\n\n\tstatic inline off_t off_t_or_die(uintmax_t val, const char *msg_if_bad)\n\t{\n\t\tif (val > maximum_signed_value_of_type(off_t))\n\t\t\tdie(\"%s\", msg_if_bad);\n\t\treturn (off_t) val;\n\t}\n\n\t...\n\n\t\toff_t delta_len = off_t_or_die(len, \"enormous delta\");\n\t\tpostimage_len = apply_delta(delta_len, input, ...);\n\nWhat do you think?\n\nJonathan\n"},{"id":"183429","messageId":"7v8vkn8wa1.fsf@alter.siamese.dyndns.org","threadId":"29492","inReplyTo":"20120131192053.GC12443@burratino","subject":"Re: [PATCH] vcs-svn: Fix some compiler warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-31T20:14:14Z","receivedAt":"2012-01-31T20:14:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> \t\toff_t delta_len = off_t_or_die(len, \"enormous delta\");\n> \t\tpostimage_len = apply_delta(delta_len, input, ...);\n>\n> What do you think?\n\nAnother possibility would be to make the \"die\" part responsibility of the\ncaller of the helper function, e.g.\n\n\tif (value_out_of_range(off_t, len))\n\t\tdie(\"enormous delta\");\n\nwhich may make the caller easier to follow and the helper easier to reuse.\n"},{"id":"183540","messageId":"7vipjpzxav.fsf@alter.siamese.dyndns.org","threadId":"29492","inReplyTo":"20120131192053.GC12443@burratino","subject":"Re: [PATCH] vcs-svn: Fix some compiler warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-02T04:14:32Z","receivedAt":"2012-02-02T04:14:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Thanks for tracking this down.  Interesting.  -Wtype-limits was split\n> out from the default set of warnings (!) in gcc 4.3 to address\n> <http://gcc.gnu.org/PR12963>, among other bugs (r124875, 2007-05-20).\n>\n> [...]\n>> --- a/vcs-svn/fast_export.c\n>> +++ b/vcs-svn/fast_export.c\n>> @@ -300,7 +300,8 @@ void fast_export_blob_delta(uint32_t mode,\n>>  \t\t\t\tuint32_t len, struct line_buffer *input)\n>>  {\n>>  \tlong postimage_len;\n>> -\tif (len > maximum_signed_value_of_type(off_t))\n>> +\tuintmax_t delta_len = (uintmax_t) len;\n>> +\tif (delta_len > maximum_signed_value_of_type(off_t))\n>>  \t\tdie(\"enormous delta\");\n>>  \tpostimage_len = apply_delta((off_t) len, input, old_data, old_mode);\n>\n> Is there some less ugly way to write the condition \"if this value is\n> not representable in this type\"?\n> \n> I guess I could live with something like the following (please don't\n> take the names too seriously):\n> ...\n> What do you think?\n\nI'd hold the branch in 'next' for now, until this gets resolved (one\npossible resolution is to declare Ramsey's patch is good enough for now,\nand do the follow-up later).\n"},{"id":"183577","messageId":"20120202104128.GG3823@burratino","threadId":"29492","inReplyTo":"7vipjpzxav.fsf@alter.siamese.dyndns.org","subject":"[PATCH/RFC 0/3] Re: [PATCH] vcs-svn: Fix some compiler warnings","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-02T10:41:28Z","receivedAt":"2012-02-02T10:41:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> I'd hold the branch in 'next' for now, until this gets resolved (one\n> possible resolution is to declare Ramsey's patch is good enough for now,\n> and do the follow-up later).\n\nSounds sensible.  How about the following?  Intended to replace\nRamsay's patch.  Not well tested yet.\n\nBy the way, wouldn't the (p->field < 0) test in grep.c line 330\ntrigger the same compiler bug?\n\nJonathan Nieder (2):\n  vcs-svn: allow import of > 4GiB files\n  vcs-svn: suppress a -Wtype-limits warning\n\nRamsay Allan Jones (1):\n  vcs-svn: rename check_overflow arguments for clarity\n\n vcs-svn/fast_export.c    |   15 +++++++++------\n vcs-svn/fast_export.h    |    4 ++--\n vcs-svn/sliding_window.c |   10 +++++-----\n vcs-svn/svndump.c        |   21 +++++++++++++++------\n 4 files changed, 31 insertions(+), 19 deletions(-)\n"},{"id":"183580","messageId":"20120202105923.GJ3823@burratino","threadId":"29492","inReplyTo":"20120202104128.GG3823@burratino","subject":"[PATCH 1/3] vcs-svn: rename check_overflow arguments for clarity","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-02T10:59:23Z","receivedAt":"2012-02-02T10:59:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n\nCode using the argument names a and b just doesn't look right (not\nsure why!).  Use more explicit names \"offset\" and \"len\" to make their\ntype and function clearer.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nSplit out from Ramsay's patch.\n\n vcs-svn/sliding_window.c |   10 +++++-----\n 1 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c\nindex 1bac7a4c..fafa4a63 100644\n--- a/vcs-svn/sliding_window.c\n+++ b/vcs-svn/sliding_window.c\n@@ -31,15 +31,15 @@ static int read_to_fill_or_whine(struct line_buffer *file,\n \treturn 0;\n }\n \n-static int check_overflow(off_t a, size_t b)\n+static int check_overflow(off_t offset, size_t len)\n {\n-\tif (b > maximum_signed_value_of_type(off_t))\n+\tif (len > maximum_signed_value_of_type(off_t))\n \t\treturn error(\"unrepresentable length in delta: \"\n-\t\t\t\t\"%\"PRIuMAX\" > OFF_MAX\", (uintmax_t) b);\n+\t\t\t\t\"%\"PRIuMAX\" > OFF_MAX\", (uintmax_t) len);\n-\tif (signed_add_overflows(a, (off_t) b))\n+\tif (signed_add_overflows(offset, (off_t) len))\n \t\treturn error(\"unrepresentable offset in delta: \"\n \t\t\t\t\"%\"PRIuMAX\" + %\"PRIuMAX\" > OFF_MAX\",\n-\t\t\t\t(uintmax_t) a, (uintmax_t) b);\n+\t\t\t\t(uintmax_t) offset, (uintmax_t) len);\n \treturn 0;\n }\n \n-- \n1.7.9\n"},{"id":"183583","messageId":"20120202110316.GK3823@burratino","threadId":"29492","inReplyTo":"20120202104128.GG3823@burratino","subject":"[PATCH 2/3] vcs-svn: allow import of > 4GiB files","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-02T11:03:16Z","receivedAt":"2012-02-02T11:03:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"There is no reason in principle that an svn-format dump would not be\nable to represent a file whose length does not fit in a 32-bit\ninteger.  Use off_t consistently to represent file lengths (in place\nof using uint32_t in some contexts) so we can handle that.\n\nMost svn-fe code is already ready to do that without this patch and\npasses values of type off_t around.  The type mismatch from stragglers\nwas noticed with gcc -Wtype-limits.\n\nWhile at it, tighten the parsing of the Text-content-length field to\nmake sure it is a number and does not overflow, and tighten other\noverflow checks as that value is passed around and manipulated.\n\nInspired-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n vcs-svn/fast_export.c |   15 +++++++++------\n vcs-svn/fast_export.h |    4 ++--\n vcs-svn/svndump.c     |   21 +++++++++++++++------\n 3 files changed, 26 insertions(+), 14 deletions(-)\n\ndiff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c\nindex 19d7c34c..b823b851 100644\n--- a/vcs-svn/fast_export.c\n+++ b/vcs-svn/fast_export.c\n@@ -227,15 +227,18 @@ static long apply_delta(off_t len, struct line_buffer *input,\n \treturn ret;\n }\n \n-void fast_export_data(uint32_t mode, uint32_t len, struct line_buffer *input)\n+void fast_export_data(uint32_t mode, off_t len, struct line_buffer *input)\n {\n+\tassert(len >= 0);\n \tif (mode == REPO_MODE_LNK) {\n \t\t/* svn symlink blobs start with \"link \" */\n+\t\tif (len < 5)\n+\t\t\tdie(\"invalid dump: symlink too short for \\\"link\\\" prefix\");\n \t\tlen -= 5;\n \t\tif (buffer_skip_bytes(input, 5) != 5)\n \t\t\tdie_short_read(input);\n \t}\n-\tprintf(\"data %\"PRIu32\"\\n\", len);\n+\tprintf(\"data %\"PRIuMAX\"\\n\", (uintmax_t) len);\n \tif (buffer_copy_bytes(input, len) != len)\n \t\tdie_short_read(input);\n \tfputc('\\n', stdout);\n@@ -297,12 +300,12 @@ int fast_export_ls(const char *path, uint32_t *mode, struct strbuf *dataref)\n \n void fast_export_blob_delta(uint32_t mode,\n \t\t\t\tuint32_t old_mode, const char *old_data,\n-\t\t\t\tuint32_t len, struct line_buffer *input)\n+\t\t\t\toff_t len, struct line_buffer *input)\n {\n \tlong postimage_len;\n-\tif (len > maximum_signed_value_of_type(off_t))\n-\t\tdie(\"enormous delta\");\n-\tpostimage_len = apply_delta((off_t) len, input, old_data, old_mode);\n+\n+\tassert(len >= 0);\n+\tpostimage_len = apply_delta(len, input, old_data, old_mode);\n \tif (mode == REPO_MODE_LNK) {\n \t\tbuffer_skip_bytes(&postimage, strlen(\"link \"));\n \t\tpostimage_len -= strlen(\"link \");\ndiff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h\nindex 43d05b65..aa629f54 100644\n--- a/vcs-svn/fast_export.h\n+++ b/vcs-svn/fast_export.h\n@@ -14,10 +14,10 @@ void fast_export_begin_commit(uint32_t revision, const char *author,\n \t\t\tconst struct strbuf *log, const char *uuid,\n \t\t\tconst char *url, unsigned long timestamp);\n void fast_export_end_commit(uint32_t revision);\n-void fast_export_data(uint32_t mode, uint32_t len, struct line_buffer *input);\n+void fast_export_data(uint32_t mode, off_t len, struct line_buffer *input);\n void fast_export_blob_delta(uint32_t mode,\n \t\t\tuint32_t old_mode, const char *old_data,\n-\t\t\tuint32_t len, struct line_buffer *input);\n+\t\t\toff_t len, struct line_buffer *input);\n \n /* If there is no such file at that rev, returns -1, errno == ENOENT. */\n int fast_export_ls_rev(uint32_t rev, const char *path,\ndiff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c\nindex ca63760f..644fdc71 100644\n--- a/vcs-svn/svndump.c\n+++ b/vcs-svn/svndump.c\n@@ -40,7 +40,8 @@\n static struct line_buffer input = LINE_BUFFER_INIT;\n \n static struct {\n-\tuint32_t action, propLength, textLength, srcRev, type;\n+\tuint32_t action, propLength, srcRev, type;\n+\toff_t text_length;\n \tstruct strbuf src, dst;\n \tuint32_t text_delta, prop_delta;\n } node_ctx;\n@@ -61,7 +62,7 @@ static void reset_node_ctx(char *fname)\n \tnode_ctx.type = 0;\n \tnode_ctx.action = NODEACT_UNKNOWN;\n \tnode_ctx.propLength = LENGTH_UNKNOWN;\n-\tnode_ctx.textLength = LENGTH_UNKNOWN;\n+\tnode_ctx.text_length = -1;\n \tstrbuf_reset(&node_ctx.src);\n \tnode_ctx.srcRev = 0;\n \tstrbuf_reset(&node_ctx.dst);\n@@ -209,7 +210,7 @@ static void handle_node(void)\n {\n \tconst uint32_t type = node_ctx.type;\n \tconst int have_props = node_ctx.propLength != LENGTH_UNKNOWN;\n-\tconst int have_text = node_ctx.textLength != LENGTH_UNKNOWN;\n+\tconst int have_text = node_ctx.text_length != -1;\n \t/*\n \t * Old text for this node:\n \t *  NULL\t- directory or bug\n@@ -291,12 +292,12 @@ static void handle_node(void)\n \t}\n \tif (!node_ctx.text_delta) {\n \t\tfast_export_modify(node_ctx.dst.buf, node_ctx.type, \"inline\");\n-\t\tfast_export_data(node_ctx.type, node_ctx.textLength, &input);\n+\t\tfast_export_data(node_ctx.type, node_ctx.text_length, &input);\n \t\treturn;\n \t}\n \tfast_export_modify(node_ctx.dst.buf, node_ctx.type, \"inline\");\n \tfast_export_blob_delta(node_ctx.type, old_mode, old_data,\n-\t\t\t\tnode_ctx.textLength, &input);\n+\t\t\t\tnode_ctx.text_length, &input);\n }\n \n static void begin_revision(void)\n@@ -409,7 +410,15 @@ void svndump_read(const char *url)\n \t\t\tbreak;\n \t\tcase sizeof(\"Text-content-length\"):\n \t\t\tif (!constcmp(t, \"Text-content-length\")) {\n-\t\t\t\tnode_ctx.textLength = atoi(val);\n+\t\t\t\tchar *end;\n+\t\t\t\tuintmax_t textlen;\n+\n+\t\t\t\ttextlen = strtoumax(val, &end, 10);\n+\t\t\t\tif (!isdigit(*val) || *end)\n+\t\t\t\t\tdie(\"invalid dump: non-numeric length %s\", val);\n+\t\t\t\tif (textlen > maximum_signed_value_of_type(off_t))\n+\t\t\t\t\tdie(\"unrepresentable length in dump: %s\", val);\n+\t\t\t\tnode_ctx.text_length = (off_t) textlen;\n \t\t\t\tbreak;\n \t\t\t}\n \t\t\tif (constcmp(t, \"Prop-content-length\"))\n-- \n1.7.9\n"},{"id":"183584","messageId":"CA+gfSn9Exv3T0UCB-bFShmSvRCMgygVuWraiToR6ZjgOA_sZ8A@mail.gmail.com","threadId":"29492","inReplyTo":"20120202105923.GJ3823@burratino","subject":"Re: [PATCH 1/3] vcs-svn: rename check_overflow arguments for clarity","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2012-02-02T11:05:23Z","receivedAt":"2012-02-02T11:05:23Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"Hi\n\nOn Thu, Feb 2, 2012 at 4:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n>\n> Code using the argument names a and b just doesn't look right (not\n> sure why!).  Use more explicit names \"offset\" and \"len\" to make their\n> type and function clearer.\nWell, it's still not clear. Given off_t a, size_t b, check that a+b\nfits into type... which type?\n\"offset\" and \"length\" don't imply that it's \"type of offset\" or maybe\n\"type of length\".\n\n>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> Split out from Ramsay's patch.\n>\n>  vcs-svn/sliding_window.c |   10 +++++-----\n>  1 files changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c\n> index 1bac7a4c..fafa4a63 100644\n> --- a/vcs-svn/sliding_window.c\n> +++ b/vcs-svn/sliding_window.c\n> @@ -31,15 +31,15 @@ static int read_to_fill_or_whine(struct line_buffer *file,\n>        return 0;\n>  }\n>\n> -static int check_overflow(off_t a, size_t b)\n> +static int check_overflow(off_t offset, size_t len)\n>  {\n> -       if (b > maximum_signed_value_of_type(off_t))\n> +       if (len > maximum_signed_value_of_type(off_t))\n>                return error(\"unrepresentable length in delta: \"\n> -                               \"%\"PRIuMAX\" > OFF_MAX\", (uintmax_t) b);\n> +                               \"%\"PRIuMAX\" > OFF_MAX\", (uintmax_t) len);\n> -       if (signed_add_overflows(a, (off_t) b))\n> +       if (signed_add_overflows(offset, (off_t) len))\n>                return error(\"unrepresentable offset in delta: \"\n>                                \"%\"PRIuMAX\" + %\"PRIuMAX\" > OFF_MAX\",\n> -                               (uintmax_t) a, (uintmax_t) b);\n> +                               (uintmax_t) offset, (uintmax_t) len);\n>        return 0;\n>  }\n>\n> --\n> 1.7.9\n>\n"},{"id":"183585","messageId":"20120202110601.GL3823@burratino","threadId":"29492","inReplyTo":"20120202104128.GG3823@burratino","subject":"[PATCH 3/3] vcs-svn: suppress a -Wtype-limits warning","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-02T11:06:01Z","receivedAt":"2012-02-02T11:06:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"On 32-bit architectures with 64-bit file offsets, gcc 4.3 and earlier\nproduce the following warning:\n\n\t    CC vcs-svn/sliding_window.o\n\tvcs-svn/sliding_window.c: In function `check_overflow':\n\tvcs-svn/sliding_window.c:36: warning: comparison is always false \\\n\t    due to limited range of data type\n\nThe warning appears even when gcc is run without any warning flags\n(this is gcc bug 12963).  In later versions the same warning can be\nreproduced with -Wtype-limits, which is implied by -Wextra.\n\nOn 64-bit architectures it really is possible for a size_t not to be\nrepresentable as an off_t so the check this is warning about is not\nactually redundant.  But even false positives are distracting.  Avoid\nthe warning by making the \"len\" argument to check_overflow a\nuintmax_t; no functional change intended.\n\nReported-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nThat's the end of the series.  I hope it was entertaining.\n\nThoughts of all kinds welcome, as usual.\n\nJonathan\n\n vcs-svn/sliding_window.c |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c\nindex fafa4a63..2f4ae60f 100644\n--- a/vcs-svn/sliding_window.c\n+++ b/vcs-svn/sliding_window.c\n@@ -31,15 +31,15 @@ static int read_to_fill_or_whine(struct line_buffer *file,\n \treturn 0;\n }\n \n-static int check_overflow(off_t offset, size_t len)\n+static int check_overflow(off_t offset, uintmax_t len)\n {\n \tif (len > maximum_signed_value_of_type(off_t))\n \t\treturn error(\"unrepresentable length in delta: \"\n-\t\t\t\t\"%\"PRIuMAX\" > OFF_MAX\", (uintmax_t) len);\n+\t\t\t\t\"%\"PRIuMAX\" > OFF_MAX\", len);\n \tif (signed_add_overflows(offset, (off_t) len))\n \t\treturn error(\"unrepresentable offset in delta: \"\n \t\t\t\t\"%\"PRIuMAX\" + %\"PRIuMAX\" > OFF_MAX\",\n-\t\t\t\t(uintmax_t) offset, (uintmax_t) len);\n+\t\t\t\t(uintmax_t) offset, len);\n \treturn 0;\n }\n \n-- \n1.7.9\n"},{"id":"183587","messageId":"20120202111628.GN3823@burratino","threadId":"29492","inReplyTo":"CA+gfSn9Exv3T0UCB-bFShmSvRCMgygVuWraiToR6ZjgOA_sZ8A@mail.gmail.com","subject":"Re: [PATCH 1/3] vcs-svn: rename check_overflow arguments for clarity","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-02T11:16:28Z","receivedAt":"2012-02-02T11:16:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Dmitry Ivankov wrote:\n> On Thu, Feb 2, 2012 at 4:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n>> From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n>>\n>> Code using the argument names a and b just doesn't look right (not\n>> sure why!).  Use more explicit names \"offset\" and \"len\" to make their\n>> type and function clearer.\n>\n> Well, it's still not clear. Given off_t a, size_t b, check that a+b\n> fits into type... which type?\n> \"offset\" and \"length\" don't imply that it's \"type of offset\" or maybe\n> \"type of length\".\n\nHmm... in vector arithmetic, position (i.e., file offset) + displacement\n(i.e., size of chunk) = position (i.e., new file offset).  Any ideas\nfor making this clearer?\n"},{"id":"183589","messageId":"CAFfmPPOeFk871m_N+nLXgQx3Uj4wVhgR9BNFzM2ggtseop0JaA@mail.gmail.com","threadId":"29492","inReplyTo":"20120202111628.GN3823@burratino","subject":"Re: [PATCH 1/3] vcs-svn: rename check_overflow arguments for clarity","fromName":"David Barr","fromEmail":"davidbarr@google.com","sentAt":"2012-02-02T11:25:31Z","receivedAt":"2012-02-02T11:25:31Z","isPatch":true,"sender":{"key":"davidbarr@google.com","avatar":"https://avatars.githubusercontent.com/u/220594?v=4"},"body":"On Thu, Feb 2, 2012 at 10:16 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Dmitry Ivankov wrote:\n>> On Thu, Feb 2, 2012 at 4:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n>\n>>> From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n>>>\n>>> Code using the argument names a and b just doesn't look right (not\n>>> sure why!).  Use more explicit names \"offset\" and \"len\" to make their\n>>> type and function clearer.\n>>\n>> Well, it's still not clear. Given off_t a, size_t b, check that a+b\n>> fits into type... which type?\n>> \"offset\" and \"length\" don't imply that it's \"type of offset\" or maybe\n>> \"type of length\".\n>\n> Hmm... in vector arithmetic, position (i.e., file offset) + displacement\n> (i.e., size of chunk) = position (i.e., new file offset).  Any ideas\n> for making this clearer?\n\nMaybe rename to check_offset_overflow to make it explicit?\n--\nDavid Barr\n"},{"id":"183590","messageId":"20120202112732.GA15537@burratino","threadId":"29492","inReplyTo":"CAFfmPPOeFk871m_N+nLXgQx3Uj4wVhgR9BNFzM2ggtseop0JaA@mail.gmail.com","subject":"Re: [PATCH 1/3] vcs-svn: rename check_overflow arguments for clarity","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-02T11:27:32Z","receivedAt":"2012-02-02T11:27:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"David Barr wrote:\n\n> Maybe rename to check_offset_overflow to make it explicit?\n\nOk, here's the patch I'll squash in. ;-)\n\ndiff --git i/vcs-svn/sliding_window.c w/vcs-svn/sliding_window.c\nindex 2f4ae60f..ec2707c9 100644\n--- i/vcs-svn/sliding_window.c\n+++ w/vcs-svn/sliding_window.c\n@@ -31,7 +31,7 @@ static int read_to_fill_or_whine(struct line_buffer *file,\n \treturn 0;\n }\n \n-static int check_overflow(off_t offset, uintmax_t len)\n+static int check_offset_overflow(off_t offset, uintmax_t len)\n {\n \tif (len > maximum_signed_value_of_type(off_t))\n \t\treturn error(\"unrepresentable length in delta: \"\n@@ -48,9 +48,9 @@ int move_window(struct sliding_view *view, off_t off, size_t width)\n \toff_t file_offset;\n \tassert(view);\n \tassert(view->width <= view->buf.len);\n-\tassert(!check_overflow(view->off, view->buf.len));\n+\tassert(!check_offset_overflow(view->off, view->buf.len));\n \n-\tif (check_overflow(off, width))\n+\tif (check_offset_overflow(off, width))\n \t\treturn -1;\n \tif (off < view->off || off + width < view->off + view->width)\n \t\treturn error(\"invalid delta: window slides left\");\n"},{"id":"183624","messageId":"4F2AD4CF.7020303@ramsay1.demon.co.uk","threadId":"29492","inReplyTo":"20120131192053.GC12443@burratino","subject":"Re: [PATCH] vcs-svn: Fix some compiler warnings","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-02-02T18:24:15Z","receivedAt":"2012-02-02T18:24:15Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Jonathan Nieder wrote:\n> Ramsay Jones wrote:\n> \n>> In particular, some versions of gcc complains as follows:\n>>\n>>         CC vcs-svn/sliding_window.o\n>>     vcs-svn/sliding_window.c: In function `check_overflow':\n>>     vcs-svn/sliding_window.c:36: warning: comparison is always false \\\n>>         due to limited range of data type\n> \n> Yuck.  Suppressing this warning would presumably also suppress the\n> optimization that notices the comparison is always false.\n\nI didn't check, but I would assume so.  I would prefer not to loose any\nchance at optimizing the code, but in this case, given that we are talking\nabout a fatal error path, I don't think it is much of a loss.\n\n> The -Wtype-limits warning also triggers in some other perfectly\n> reasonable situations: see <http://gcc.gnu.org/PR51712>.\n[...]\n>> Note that the \"some versions of gcc\" which complain includes 3.4.4 and\n>> 4.1.2, whereas gcc version 4.4.0 compiles the code without complaint.\n> \n> Thanks for tracking this down.  Interesting.  -Wtype-limits was split\n> out from the default set of warnings (!) in gcc 4.3 to address\n> <http://gcc.gnu.org/PR12963>, among other bugs (r124875, 2007-05-20).\n\nThanks for the above references, and for taking the time to track them\ndown.\n\n> Is there some less ugly way to write the condition \"if this value is\n> not representable in this type\"?\n> \n> I guess I could live with something like the following (please don't\n> take the names too seriously):\n> \n> \tstatic inline off_t off_t_or_die(uintmax_t val, const char *msg_if_bad)\n> \t{\n> \t\tif (val > maximum_signed_value_of_type(off_t))\n> \t\t\tdie(\"%s\", msg_if_bad);\n> \t\treturn (off_t) val;\n> \t}\n> \n> \t...\n> \n> \t\toff_t delta_len = off_t_or_die(len, \"enormous delta\");\n> \t\tpostimage_len = apply_delta(delta_len, input, ...);\n> \n> What do you think?\n\nAn static inline function was actually my first thought (although I had\nsomething more like Junio's suggestion [elsewhere in this thread] in mind),\nbut I didn't want to place it in git-compat-util.h and could not find a\nsuitable place in the vcs-svn directory.\n\nHmm, I will send a v2 patch along these lines ...\n\nATB,\nRamsay Jones\n"},{"id":"183628","messageId":"20120202185305.GB19520@burratino","threadId":"29492","inReplyTo":"4F2AD4CF.7020303@ramsay1.demon.co.uk","subject":"Re: [PATCH] vcs-svn: Fix some compiler warnings","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-02-02T18:53:05Z","receivedAt":"2012-02-02T18:53:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramsay Jones wrote:\n\n> An static inline function was actually my first thought (although I had\n> something more like Junio's suggestion [elsewhere in this thread] in mind),\n> but I didn't want to place it in git-compat-util.h and could not find a\n> suitable place in the vcs-svn directory.\n>\n> Hmm, I will send a v2 patch along these lines ...\n\nWell, your v2 patch looks good to me.  The three-patch series I\nsent[1] also look good to me.  I guess I could queue your v2 and put\nthe rest on top --- what say you?\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/189618\nhttp://repo.or.cz/w/git/jrn.git/log/refs/topics/rj/svn-fe-type-limits\n"},{"id":"183629","messageId":"7vr4ydvzcs.fsf@alter.siamese.dyndns.org","threadId":"29492","inReplyTo":"20120202112732.GA15537@burratino","subject":"Re: [PATCH 1/3] vcs-svn: rename check_overflow arguments for clarity","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-02T18:56:03Z","receivedAt":"2012-02-02T18:56:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ok, I've tentatively queued this.\n\n-- >8 --\nFrom: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n\nCode using the argument names a and b just doesn't look right (not\nsure why!).  Use more explicit names \"offset\" and \"len\" to make their\ntype and meaning clearer.\n\nAlso rename check_overflow() to check_offset_overflow() to clarify\nthat we are making sure that \"len\" bytes beyond \"offset\" still fits\nthe type to represent an offset.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n vcs-svn/sliding_window.c |   14 +++++++-------\n 1 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c\nindex 1bac7a4..c6c2eff 100644\n--- a/vcs-svn/sliding_window.c\n+++ b/vcs-svn/sliding_window.c\n@@ -31,15 +31,15 @@ static int read_to_fill_or_whine(struct line_buffer *file,\n \treturn 0;\n }\n \n-static int check_overflow(off_t a, size_t b)\n+static int check_offset_overflow(off_t offset, size_t len)\n {\n-\tif (b > maximum_signed_value_of_type(off_t))\n+\tif (len > maximum_signed_value_of_type(off_t))\n \t\treturn error(\"unrepresentable length in delta: \"\n-\t\t\t\t\"%\"PRIuMAX\" > OFF_MAX\", (uintmax_t) b);\n-\tif (signed_add_overflows(a, (off_t) b))\n+\t\t\t\t\"%\"PRIuMAX\" > OFF_MAX\", (uintmax_t) len);\n+\tif (signed_add_overflows(offset, (off_t) len))\n \t\treturn error(\"unrepresentable offset in delta: \"\n \t\t\t\t\"%\"PRIuMAX\" + %\"PRIuMAX\" > OFF_MAX\",\n-\t\t\t\t(uintmax_t) a, (uintmax_t) b);\n+\t\t\t\t(uintmax_t) offset, (uintmax_t) len);\n \treturn 0;\n }\n \n@@ -48,9 +48,9 @@ int move_window(struct sliding_view *view, off_t off, size_t width)\n \toff_t file_offset;\n \tassert(view);\n \tassert(view->width <= view->buf.len);\n-\tassert(!check_overflow(view->off, view->buf.len));\n+\tassert(!check_offset_overflow(view->off, view->buf.len));\n \n-\tif (check_overflow(off, width))\n+\tif (check_offset_overflow(off, width))\n \t\treturn -1;\n \tif (off < view->off || off + width < view->off + view->width)\n \t\treturn error(\"invalid delta: window slides left\");\n-- \n1.7.9.172.ge26ae\n"},{"id":"183675","messageId":"4F2B0BD2.2030801@ramsay1.demon.co.uk","threadId":"29492","inReplyTo":"20120202110601.GL3823@burratino","subject":"Re: [PATCH 3/3] vcs-svn: suppress a -Wtype-limits warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-02-02T22:18:58Z","receivedAt":"2012-02-02T22:18:58Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Jonathan Nieder wrote:\n> ---\n> That's the end of the series.  I hope it was entertaining.\n> \n> Thoughts of all kinds welcome, as usual.\n\nYes, this is a much better approach! Thanks!\n\nI've only compile tested (on cygwin and mingw) so far, but\nI don't expect any problems ...\n\nSo, please disregard my earlier v2 patch. [If it's not already\nobvious, I often don't read the list every day and I missed\nall of the discussion which resulted in this series (while, at\nthe same time, writing testing and sending the v2 patch!).]\n\nATB,\nRamsay Jones\n"}]}