{"thread":{"id":"49752","subject":"[PATCH] multi-pack-index: make code -Wunused-parameter clean","startedAt":"2018-11-04T00:50:38Z","lastAt":"2018-11-05T16:48:22Z","messageCount":3,"participants":["Carlo Marcelo Arenas Belón","Jeff King","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"362379","messageId":"20181104004957.52913-1-carenas@gmail.com","threadId":"49752","inReplyTo":null,"subject":"[PATCH] multi-pack-index: make code -Wunused-parameter clean","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2018-11-04T00:49:57Z","receivedAt":"2018-11-04T00:50:38Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"introduced in 662148c435 (\"midx: write object offsets\", 2018-07-12)\nbut included on all previous versions as well.\n\nmidx.c:713:54: warning: unused parameter 'nr_objects' [-Wunused-parameter]\n\nlikely an oversight as the information needed to iterate over is\nembedded in nr_large_offset\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n midx.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 4fac0cd08a..a2c17e3108 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -710,7 +710,7 @@ static size_t write_midx_object_offsets(struct hashfile *f, int large_offset_nee\n }\n \n static size_t write_midx_large_offsets(struct hashfile *f, uint32_t nr_large_offset,\n-\t\t\t\t       struct pack_midx_entry *objects, uint32_t nr_objects)\n+\t\t\t\t       struct pack_midx_entry *objects)\n {\n \tstruct pack_midx_entry *list = objects;\n \tsize_t written = 0;\n@@ -880,7 +880,7 @@ int write_midx_file(const char *object_dir)\n \t\t\t\tbreak;\n \n \t\t\tcase MIDX_CHUNKID_LARGEOFFSETS:\n-\t\t\t\twritten += write_midx_large_offsets(f, num_large_offsets, entries, nr_entries);\n+\t\t\t\twritten += write_midx_large_offsets(f, num_large_offsets, entries);\n \t\t\t\tbreak;\n \n \t\t\tdefault:\n-- \n2.19.1.816.gcd69ec8cd\n\n"},{"id":"362383","messageId":"20181104022746.GA30936@sigill.intra.peff.net","threadId":"49752","inReplyTo":"20181104004957.52913-1-carenas@gmail.com","subject":"Re: [PATCH] multi-pack-index: make code -Wunused-parameter clean","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-04T02:27:46Z","receivedAt":"2018-11-04T02:34:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 03, 2018 at 05:49:57PM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> introduced in 662148c435 (\"midx: write object offsets\", 2018-07-12)\n> but included on all previous versions as well.\n> \n> midx.c:713:54: warning: unused parameter 'nr_objects' [-Wunused-parameter]\n> \n> likely an oversight as the information needed to iterate over is\n> embedded in nr_large_offset\n\nI've been preparing a series to make the whole code base compile with\n-Wunused-parameter, and I handled this case a bit differently.\n\n-- >8 --\nSubject: [PATCH] midx: double-check large object write loop\n\nThe write_midx_large_offsets() function takes an array of object\nentries, the number of entries in the array (nr_objects), and the number\nof entries with large offsets (nr_large_offset). But we never actually\nuse nr_objects; instead we keep walking down the array and counting down\nnr_large_offset until we've seen all of the large entries.\n\nThis is correct, but we can be a bit more defensive. If there were ever\na mismatch between nr_large_offset and the actual set of large-offset\nobjects, we'd walk off the end of the array.\n\nSince we know the size of the array, we can use nr_objects to make sure\nwe don't walk too far.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n midx.c | 12 +++++++++---\n 1 file changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 4fac0cd08a..ecd583666a 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -712,12 +712,18 @@ static size_t write_midx_object_offsets(struct hashfile *f, int large_offset_nee\n static size_t write_midx_large_offsets(struct hashfile *f, uint32_t nr_large_offset,\n \t\t\t\t       struct pack_midx_entry *objects, uint32_t nr_objects)\n {\n-\tstruct pack_midx_entry *list = objects;\n+\tstruct pack_midx_entry *list = objects, *end = objects + nr_objects;\n \tsize_t written = 0;\n \n \twhile (nr_large_offset) {\n-\t\tstruct pack_midx_entry *obj = list++;\n-\t\tuint64_t offset = obj->offset;\n+\t\tstruct pack_midx_entry *obj;\n+\t\tuint64_t offset;\n+\n+\t\tif (list >= end)\n+\t\t\tBUG(\"too many large-offset objects\");\n+\n+\t\tobj = list++;\n+\t\toffset = obj->offset;\n \n \t\tif (!(offset >> 31))\n \t\t\tcontinue;\n-- \n2.19.1.1352.g60f3b1a4c2\n\n"},{"id":"362492","messageId":"0ace7b35-e967-6230-8733-e9cacebc75c1@gmail.com","threadId":"49752","inReplyTo":"20181104022746.GA30936@sigill.intra.peff.net","subject":"Re: [PATCH] multi-pack-index: make code -Wunused-parameter clean","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-11-05T16:48:17Z","receivedAt":"2018-11-05T16:48:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/3/2018 10:27 PM, Jeff King wrote:\n> On Sat, Nov 03, 2018 at 05:49:57PM -0700, Carlo Marcelo Arenas Belón wrote:\n>\n>> introduced in 662148c435 (\"midx: write object offsets\", 2018-07-12)\n>> but included on all previous versions as well.\n>>\n>> midx.c:713:54: warning: unused parameter 'nr_objects' [-Wunused-parameter]\n>>\n>> likely an oversight as the information needed to iterate over is\n>> embedded in nr_large_offset\n> I've been preparing a series to make the whole code base compile with\n> -Wunused-parameter, and I handled this case a bit differently.\n>\n> -- >8 --\n> Subject: [PATCH] midx: double-check large object write loop\n>\n> The write_midx_large_offsets() function takes an array of object\n> entries, the number of entries in the array (nr_objects), and the number\n> of entries with large offsets (nr_large_offset). But we never actually\n> use nr_objects; instead we keep walking down the array and counting down\n> nr_large_offset until we've seen all of the large entries.\n>\n> This is correct, but we can be a bit more defensive. If there were ever\n> a mismatch between nr_large_offset and the actual set of large-offset\n> objects, we'd walk off the end of the array.\n>\n> Since we know the size of the array, we can use nr_objects to make sure\n> we don't walk too far.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nThanks, both, for catching this. I prefer the approach that adds defenses.\n\nReviewed-by: Derrick Stolee <dstolee@microsoft.com>\n\n> ---\n>   midx.c | 12 +++++++++---\n>   1 file changed, 9 insertions(+), 3 deletions(-)\n>\n> diff --git a/midx.c b/midx.c\n> index 4fac0cd08a..ecd583666a 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -712,12 +712,18 @@ static size_t write_midx_object_offsets(struct hashfile *f, int large_offset_nee\n>   static size_t write_midx_large_offsets(struct hashfile *f, uint32_t nr_large_offset,\n>   \t\t\t\t       struct pack_midx_entry *objects, uint32_t nr_objects)\n>   {\n> -\tstruct pack_midx_entry *list = objects;\n> +\tstruct pack_midx_entry *list = objects, *end = objects + nr_objects;\n>   \tsize_t written = 0;\n>   \n>   \twhile (nr_large_offset) {\n> -\t\tstruct pack_midx_entry *obj = list++;\n> -\t\tuint64_t offset = obj->offset;\n> +\t\tstruct pack_midx_entry *obj;\n> +\t\tuint64_t offset;\n> +\n> +\t\tif (list >= end)\n> +\t\t\tBUG(\"too many large-offset objects\");\n> +\n> +\t\tobj = list++;\n> +\t\toffset = obj->offset;\n>   \n>   \t\tif (!(offset >> 31))\n>   \t\t\tcontinue;\n"}]}