{"thread":{"id":"43801","subject":"[PATCH] correct FLEXPTR_* example in comment","startedAt":"2016-08-13T09:01:44Z","lastAt":"2016-08-14T10:36:49Z","messageCount":5,"participants":["René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"298954","messageId":"57AEE1E1.3080901@web.de","threadId":"43801","inReplyTo":null,"subject":"[PATCH] correct FLEXPTR_* example in comment","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-08-13T09:01:21Z","receivedAt":"2016-08-13T09:01:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"This section is about \"The FLEXPTR_* variants\", so use FLEXPTR_ALLOC_STR\nin the example.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n git-compat-util.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 590bfdd..f52e00b 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -815,7 +815,7 @@ extern FILE *fopen_for_writing(const char *path);\n  * you can do:\n  *\n  *   struct foo *f;\n- *   FLEX_ALLOC_STR(f, name, src);\n+ *   FLEXPTR_ALLOC_STR(f, name, src);\n  *\n  * and \"name\" will point to a block of memory after the struct, which will be\n  * freed along with the struct (but the pointer can be repointed anywhere).\n-- \n2.9.3\n\n"},{"id":"298957","messageId":"20160813090947.uj6hurji5o6cwxge@sigill.intra.peff.net","threadId":"43801","inReplyTo":"57AEE1E1.3080901@web.de","subject":"Re: [PATCH] correct FLEXPTR_* example in comment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-13T09:09:48Z","receivedAt":"2016-08-13T09:09:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 13, 2016 at 11:01:21AM +0200, René Scharfe wrote:\n\n> This section is about \"The FLEXPTR_* variants\", so use FLEXPTR_ALLOC_STR\n> in the example.\n\nOops, yeah. Your patch is clearly an improvement.\n\nSince this is obviously an easy mistake to make (using one form rather\nthan the other), I wondered if the compiler would catch it.\n\nI think it would catch an accidental use of FLEXPTR instead of FLEX,\nbecause it involves an attempted assignment of an array. But I don't\nthink we would catch the reverse; we'd just write the data directly on\ntop of the pointer. That would probably crash immediately at runtime, so\nif you exercise the code at all in tests, it is OK. But something to be\naware of.\n\nI suppose it could assert(sizeof((x)->flexname) == FLEX_ALLOC) or\nsomething, but I'm not sure if it is worth worrying about.\n\n-Peff\n"},{"id":"298959","messageId":"57AEE61A.4000804@web.de","threadId":"43801","inReplyTo":"20160813090947.uj6hurji5o6cwxge@sigill.intra.peff.net","subject":"Re: [PATCH] correct FLEXPTR_* example in comment","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-08-13T09:19:22Z","receivedAt":"2016-08-13T09:20:00Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.08.2016 um 11:09 schrieb Jeff King:\n> On Sat, Aug 13, 2016 at 11:01:21AM +0200, René Scharfe wrote:\n>\n>> This section is about \"The FLEXPTR_* variants\", so use FLEXPTR_ALLOC_STR\n>> in the example.\n>\n> Oops, yeah. Your patch is clearly an improvement.\n>\n> Since this is obviously an easy mistake to make (using one form rather\n> than the other), I wondered if the compiler would catch it.\n>\n> I think it would catch an accidental use of FLEXPTR instead of FLEX,\n> because it involves an attempted assignment of an array. But I don't\n> think we would catch the reverse; we'd just write the data directly on\n> top of the pointer. That would probably crash immediately at runtime, so\n> if you exercise the code at all in tests, it is OK. But something to be\n> aware of.\n>\n> I suppose it could assert(sizeof((x)->flexname) == FLEX_ALLOC) or\n> something, but I'm not sure if it is worth worrying about.\n\nA compilation error or warning would be nice.  I scratched my head quite \na bit before figuring out that the example was wrong.  Copy&paste from a \nreputable source must be correct, right? :)\n\nRené\n\n"},{"id":"298977","messageId":"57AF582A.3050406@web.de","threadId":"43801","inReplyTo":"20160813090947.uj6hurji5o6cwxge@sigill.intra.peff.net","subject":"Re: [PATCH] correct FLEXPTR_* example in comment","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-08-13T17:26:02Z","receivedAt":"2016-08-13T17:26:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.08.2016 um 11:09 schrieb Jeff King:\n> On Sat, Aug 13, 2016 at 11:01:21AM +0200, René Scharfe wrote:\n>\n>> This section is about \"The FLEXPTR_* variants\", so use FLEXPTR_ALLOC_STR\n>> in the example.\n>\n> Oops, yeah. Your patch is clearly an improvement.\n>\n> Since this is obviously an easy mistake to make (using one form rather\n> than the other), I wondered if the compiler would catch it.\n>\n> I think it would catch an accidental use of FLEXPTR instead of FLEX,\n> because it involves an attempted assignment of an array.\n\nGcc 5.4 reports \"invalid use of flexible array member\".\n\n> But I don't\n> think we would catch the reverse; we'd just write the data directly on\n> top of the pointer. That would probably crash immediately at runtime, so\n> if you exercise the code at all in tests, it is OK. But something to be\n> aware of.\n\nNo hint at compile time, segfaults at runtime.\n\n> I suppose it could assert(sizeof((x)->flexname) == FLEX_ALLOC) or\n> something, but I'm not sure if it is worth worrying about.\n\nYou can't use sizeof with an actual flexible array.  It only works if \nFLEX_ARRAY is defined as 1 (for platforms without native support), and \nperhaps also if it's 0.\n\noffsetof(struct x, arr) == sizeof(struct x) won't work either because of \npadding.\n\nI have no idea what you can do with a flexible array that would throw a \ncompile error when done with a pointer.\n\nRené\n"},{"id":"299265","messageId":"20160813183640.axpnjj44bteraevi@sigill.intra.peff.net","threadId":"43801","inReplyTo":"57AF582A.3050406@web.de","subject":"Re: [PATCH] correct FLEXPTR_* example in comment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-13T18:36:40Z","receivedAt":"2016-08-14T10:36:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 13, 2016 at 07:26:02PM +0200, René Scharfe wrote:\n\n> > I suppose it could assert(sizeof((x)->flexname) == FLEX_ALLOC) or\n> > something, but I'm not sure if it is worth worrying about.\n> \n> You can't use sizeof with an actual flexible array.  It only works if\n> FLEX_ARRAY is defined as 1 (for platforms without native support), and\n> perhaps also if it's 0.\n> \n> offsetof(struct x, arr) == sizeof(struct x) won't work either because of\n> padding.\n> \n> I have no idea what you can do with a flexible array that would throw a\n> compile error when done with a pointer.\n\nThanks for the input. I'd say we should not worry about it. The reason\nthis particular \"bug\" persisted is because it was in a comment. People\ntend to notice code that cannot possibly do anything but segfault before\nthey even send in patches.\n\n-Peff\n"}]}