{"thread":{"id":"44727","subject":"[PATCH] mailinfo.c: move side-effects outside of assert","startedAt":"2016-12-17T19:54:29Z","lastAt":"2016-12-22T17:57:33Z","messageCount":18,"participants":["Kyle J. McKay","Johannes Schindelin","Jeff King","Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"307984","messageId":"900a55073f78a9f19daca67e468d334@3c843fe6ba8f3c586a21345a2783aa0","threadId":"44727","inReplyTo":null,"subject":"[PATCH] mailinfo.c: move side-effects outside of assert","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2016-12-17T19:54:18Z","receivedAt":"2016-12-17T19:54:29Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"Since 6b4b013f18 (mailinfo: handle in-body header continuations,\n2016-09-20, v2.11.0) mailinfo.c has contained new code with an\nassert of the form:\n\n\tassert(call_a_function(...))\n\nThe function in question, check_header, has side effects. This\nmeans that when NDEBUG is defined during a release build the\nfunction call is omitted entirely, the side effects do not\ntake place and tests (fortunately) start failing.\n\nMove the function call outside of the assert and assert on\nthe result of the function call instead so that the code\nstill works properly in a release build and passes the tests.\n\nSigned-off-by: Kyle J. McKay \n---\n\nNotes:\n Please include this PATCH in 2.11.x maint\n\n mailinfo.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 2fb3877e..47442fb5 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -708,9 +708,12 @@ static int is_scissors_line(const char *line)\n \n static void flush_inbody_header_accum(struct mailinfo *mi)\n {\n+\tint okay;\n+\n \tif (!mi->inbody_header_accum.len)\n \t\treturn;\n-\tassert(check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0));\n+\tokay = check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0);\n+\tassert(okay);\n \tstrbuf_reset(&mi->inbody_header_accum);\n }\n \n---\n"},{"id":"308066","messageId":"alpine.DEB.2.20.1612191844520.54750@virtualbox","threadId":"44727","inReplyTo":"900a55073f78a9f19daca67e468d334@3c843fe6ba8f3c586a21345a2783aa0","subject":"Re: [PATCH] mailinfo.c: move side-effects outside of assert","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-19T17:45:40Z","receivedAt":"2016-12-19T17:46:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 17 Dec 2016, Kyle J. McKay wrote:\n\n> Since 6b4b013f18 (mailinfo: handle in-body header continuations,\n> 2016-09-20, v2.11.0) mailinfo.c has contained new code with an\n> assert of the form:\n> \n> \tassert(call_a_function(...))\n> \n> The function in question, check_header, has side effects. This\n> means that when NDEBUG is defined during a release build the\n> function call is omitted entirely, the side effects do not\n> take place and tests (fortunately) start failing.\n> \n> Move the function call outside of the assert and assert on\n> the result of the function call instead so that the code\n> still works properly in a release build and passes the tests.\n> \n> Signed-off-by: Kyle J. McKay \n\nACK. I noticed this problem (and fixed it independently as a part of a\nhuge patch series I did not get around to submit yet) while trying to get\nGit to build correctly with Visual C.\n\nCiao,\nDscho\n"},{"id":"308088","messageId":"20161219200259.nqqyvk6c72bcoaui@sigill.intra.peff.net","threadId":"44727","inReplyTo":"900a55073f78a9f19daca67e468d334@3c843fe6ba8f3c586a21345a2783aa0","subject":"Re: [PATCH] mailinfo.c: move side-effects outside of assert","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-19T20:03:00Z","receivedAt":"2016-12-19T20:03:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 17, 2016 at 11:54:18AM -0800, Kyle J. McKay wrote:\n\n> Since 6b4b013f18 (mailinfo: handle in-body header continuations,\n> 2016-09-20, v2.11.0) mailinfo.c has contained new code with an\n> assert of the form:\n> \n> \tassert(call_a_function(...))\n> \n> The function in question, check_header, has side effects. This\n> means that when NDEBUG is defined during a release build the\n> function call is omitted entirely, the side effects do not\n> take place and tests (fortunately) start failing.\n> \n> Move the function call outside of the assert and assert on\n> the result of the function call instead so that the code\n> still works properly in a release build and passes the tests.\n> \n> Signed-off-by: Kyle J. McKay \n> ---\n> \n> Notes:\n> Please include this PATCH in 2.11.x maint\n\nThis is obviously an improvement, but it makes me wonder if we should be\ndoing:\n\n if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))\n\tdie(\"BUG: some explanation of why this can never happen\");\n\nwhich perhaps documents the intended assumptions more clearly. A comment\nregarding the side effects might also be helpful.\n\n-Peff\n"},{"id":"308089","messageId":"A916CED6-C49D-41D8-A7EE-A5FEDA641F4A@gmail.com","threadId":"44727","inReplyTo":"20161219200259.nqqyvk6c72bcoaui@sigill.intra.peff.net","subject":"Re: [PATCH] mailinfo.c: move side-effects outside of assert","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2016-12-19T20:38:26Z","receivedAt":"2016-12-19T20:39:32Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Dec 19, 2016, at 12:03, Jeff King wrote:\n\n> On Sat, Dec 17, 2016 at 11:54:18AM -0800, Kyle J. McKay wrote:\n>\n>> Since 6b4b013f18 (mailinfo: handle in-body header continuations,\n>> 2016-09-20, v2.11.0) mailinfo.c has contained new code with an\n>> assert of the form:\n>>\n>> \tassert(call_a_function(...))\n>>\n>> The function in question, check_header, has side effects. This\n>> means that when NDEBUG is defined during a release build the\n>> function call is omitted entirely, the side effects do not\n>> take place and tests (fortunately) start failing.\n>>\n>> Move the function call outside of the assert and assert on\n>> the result of the function call instead so that the code\n>> still works properly in a release build and passes the tests.\n>>\n>> Signed-off-by: Kyle J. McKay \n>> ---\n>>\n>> Notes:\n>> Please include this PATCH in 2.11.x maint\n>\n> This is obviously an improvement, but it makes me wonder if we \n> should be\n> doing:\n>\n> if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))\n> \tdie(\"BUG: some explanation of why this can never happen\");\n>\n> which perhaps documents the intended assumptions more clearly. A \n> comment\n> regarding the side effects might also be helpful.\n\nI wondered exactly the same thing myself. I was hoping Jonathan would \npipe in here with some analysis about whether this is:\n\n a) a super paranoid, just-in-case, can't really ever fail because \nby the time we get to this code we've already effectively validated \neverything that could cause check_header to return false in this case\n\n-or-\n\n b) Yeah, it could fail in the real world and it should \"die\" (and \nprobably have a test added that triggers such death)\n\n-or-\n\n c) Actually, if check_header does return false we can keep going \nwithout problem\n\n-or-\n\n d) Actually, if check_header does return false we can keep going by \nmaking a minor change that should be in the patch\n\nI assume that since Jonathan added the code he will just know the \nanswer as to which one it is and I won't have to rely on the results \nof my imaginary analysis. ;)\n\nOn Dec 19, 2016, at 09:45, Johannes Schindelin wrote:\n\n> ACK. I noticed this problem (and fixed it independently as a part of a\n> huge patch series I did not get around to submit yet) while trying \n> to get\n> Git to build correctly with Visual C.\n\nDoes this mean that Dscho and I are the only ones who add -DNDEBUG for \nrelease builds? Or are we just the only ones who actually run the \ntest suite on such builds?\n\n--Kyle\n"},{"id":"308092","messageId":"d5690ac7-ff62-99b9-7e7e-929bd7f0433b@google.com","threadId":"44727","inReplyTo":"A916CED6-C49D-41D8-A7EE-A5FEDA641F4A@gmail.com","subject":"Re: [PATCH] mailinfo.c: move side-effects outside of assert","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-12-19T20:54:15Z","receivedAt":"2016-12-19T20:55:26Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 12/19/2016 12:38 PM, Kyle J. McKay wrote:\n> On Dec 19, 2016, at 12:03, Jeff King wrote:\n>\n>> On Sat, Dec 17, 2016 at 11:54:18AM -0800, Kyle J. McKay wrote:\n>>\n>>> Since 6b4b013f18 (mailinfo: handle in-body header continuations,\n>>> 2016-09-20, v2.11.0) mailinfo.c has contained new code with an\n>>> assert of the form:\n>>>\n>>> assert(call_a_function(...))\n\nThanks for spotting this - I'm not sure how I missed that.\n\n>> This is obviously an improvement, but it makes me wonder if we should be\n>> doing:\n>>\n>> if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))\n>> die(\"BUG: some explanation of why this can never happen\");\n>>\n>> which perhaps documents the intended assumptions more clearly. A comment\n>> regarding the side effects might also be helpful.\n>\n> I wondered exactly the same thing myself. I was hoping Jonathan would\n> pipe in here with some analysis about whether this is:\n>\n> a) a super paranoid, just-in-case, can't really ever fail because by\n> the time we get to this code we've already effectively validated\n> everything that could cause check_header to return false in this case\n>\n> -or-\n>\n> b) Yeah, it could fail in the real world and it should \"die\" (and\n> probably have a test added that triggers such death)\n>\n> -or-\n>\n> c) Actually, if check_header does return false we can keep going\n> without problem\n>\n> -or-\n>\n> d) Actually, if check_header does return false we can keep going by\n> making a minor change that should be in the patch\n>\n> I assume that since Jonathan added the code he will just know the answer\n> as to which one it is and I won't have to rely on the results of my\n> imaginary analysis. ;)\n\nThe answer is \"a\". The only time that mi->inbody_header_accum is \nappended to is in check_inbody_header, and appending onto a blank \nmi->inbody_header_accum always happens when is_inbody_header is true \n(which guarantees a prefix that causes check_header to always return true).\n\nPeff's suggestion sounds reasonable to me, maybe with an error message \nlike \"BUG: inbody_header_accum, if not empty, must always contain a \nvalid in-body header\".\n"},{"id":"308094","messageId":"xmqqbmw7ocoz.fsf@gitster.mtv.corp.google.com","threadId":"44727","inReplyTo":"d5690ac7-ff62-99b9-7e7e-929bd7f0433b@google.com","subject":"Re: [PATCH] mailinfo.c: move side-effects outside of assert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-19T21:01:48Z","receivedAt":"2016-12-19T21:02:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>>> This is obviously an improvement, but it makes me wonder if we should be\n>>> doing:\n>>>\n>>> if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))\n>>> die(\"BUG: some explanation of why this can never happen\");\n>>>\n>>> which perhaps documents the intended assumptions more clearly. A comment\n>>> regarding the side effects might also be helpful.\n>>\n>> I wondered exactly the same thing myself. I was hoping Jonathan would\n>> pipe in here with some analysis about whether this is:\n>>\n>> a) a super paranoid, just-in-case, can't really ever fail because by\n>> the time we get to this code we've already effectively validated\n>> everything that could cause check_header to return false in this case\n>> ...\n> The answer is \"a\". The only time that mi->inbody_header_accum is\n> appended to is in check_inbody_header, and appending onto a blank\n> mi->inbody_header_accum always happens when is_inbody_header is true\n> (which guarantees a prefix that causes check_header to always return\n> true).\n>\n> Peff's suggestion sounds reasonable to me, maybe with an error message\n> like \"BUG: inbody_header_accum, if not empty, must always contain a\n> valid in-body header\".\n\nOK. So we do not expect it to fail, but we still do want the side\neffect of that function (i.e. accmulation into the field).\n\nSomebody care to send a final \"agreed-upon\" version?\n"},{"id":"308109","messageId":"ebaf4c892a78bc3ae614a23d87f9c0f@58437222ff6db9ee7cbe9d1a5a1ad4e","threadId":"44727","inReplyTo":"xmqqbmw7ocoz.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2] mailinfo.c: move side-effects outside of assert","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2016-12-19T23:13:00Z","receivedAt":"2016-12-19T23:14:20Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Dec 19, 2016, at 13:01, Junio C Hamano wrote:\n\n> Jonathan Tan writes:\n>\n>>>> This is obviously an improvement, but it makes me wonder if we \n>>>> should be\n>>>> doing:\n>>>>\n>>>> if (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data))\n>>>> die(\"BUG: some explanation of why this can never happen\");\n>>>>\n>>>> which perhaps documents the intended assumptions more clearly. A \n>>>> comment\n>>>> regarding the side effects might also be helpful.\n>>>\n>>> I wondered exactly the same thing myself. I was hoping Jonathan \n>>> would\n>>> pipe in here with some analysis about whether this is:\n>>>\n>>> a) a super paranoid, just-in-case, can't really ever fail because \n>>> by\n>>> the time we get to this code we've already effectively validated\n>>> everything that could cause check_header to return false in this \n>>> case\n>>> ...\n>> The answer is \"a\". The only time that mi->inbody_header_accum is\n>> appended to is in check_inbody_header, and appending onto a blank\n>> mi->inbody_header_accum always happens when is_inbody_header is true\n>> (which guarantees a prefix that causes check_header to always return\n>> true).\n>>\n>> Peff's suggestion sounds reasonable to me, maybe with an error \n>> message\n>> like \"BUG: inbody_header_accum, if not empty, must always contain a\n>> valid in-body header\".\n>\n> OK. So we do not expect it to fail, but we still do want the side\n> effect of that function (i.e. accmulation into the field).\n>\n> Somebody care to send a final \"agreed-upon\" version?\n\nYup, here it is:\n\n-- 8< --\n\nSince 6b4b013f18 (mailinfo: handle in-body header continuations,\n2016-09-20, v2.11.0) mailinfo.c has contained new code with an\nassert of the form:\n\n\tassert(call_a_function(...))\n\nThe function in question, check_header, has side effects. This\nmeans that when NDEBUG is defined during a release build the\nfunction call is omitted entirely, the side effects do not\ntake place and tests (fortunately) start failing.\n\nMove the function call outside of the assert and assert on\nthe result of the function call instead so that the code\nstill works properly in a release build and passes the tests.\n\nSince the only time that mi->inbody_header_accum is appended to is\nin check_inbody_header, and appending onto a blank\nmi->inbody_header_accum always happens when is_inbody_header is\ntrue, this guarantees a prefix that causes check_header to always\nreturn true.\n\nTherefore replace the assert with an if !check_header + DIE\ncombination to reflect this.\n\nHelped-by: Jonathan Tan \nHelped-by: Jeff King \nAcked-by: Johannes Schindelin \nSigned-off-by: Kyle J. McKay \n---\n\nNotes:\n Please include this PATCH in 2.11.x maint\n\n mailinfo.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 2fb3877e..a489d9d0 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -710,7 +710,8 @@ static void flush_inbody_header_accum(struct mailinfo *mi)\n {\n \tif (!mi->inbody_header_accum.len)\n \t\treturn;\n-\tassert(check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0));\n+\tif (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0))\n+\t\tdie(\"BUG: inbody_header_accum, if not empty, must always contain a valid in-body header\");\n \tstrbuf_reset(&mi->inbody_header_accum);\n }\n \n---\n"},{"id":"308112","messageId":"xmqqbmw7mrg4.fsf@gitster.mtv.corp.google.com","threadId":"44727","inReplyTo":"ebaf4c892a78bc3ae614a23d87f9c0f@58437222ff6db9ee7cbe9d1a5a1ad4e","subject":"Re: [PATCH v2] mailinfo.c: move side-effects outside of assert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-19T23:26:03Z","receivedAt":"2016-12-19T23:27:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" writes:\n\n>> OK. So we do not expect it to fail, but we still do want the side\n>> effect of that function (i.e. accmulation into the field).\n>>\n>> Somebody care to send a final \"agreed-upon\" version?\n>\n> Yup, here it is:\n\nThanks.\n\n> -- 8< --\n>\n> Since 6b4b013f18 (mailinfo: handle in-body header continuations,\n> 2016-09-20, v2.11.0) mailinfo.c has contained new code with an\n> assert of the form:\n>\n> \tassert(call_a_function(...))\n>\n> The function in question, check_header, has side effects. This\n> means that when NDEBUG is defined during a release build the\n> function call is omitted entirely, the side effects do not\n> take place and tests (fortunately) start failing.\n>\n> Move the function call outside of the assert and assert on\n> the result of the function call instead so that the code\n> still works properly in a release build and passes the tests.\n>\n> Since the only time that mi->inbody_header_accum is appended to is\n> in check_inbody_header, and appending onto a blank\n> mi->inbody_header_accum always happens when is_inbody_header is\n> true, this guarantees a prefix that causes check_header to always\n> return true.\n>\n> Therefore replace the assert with an if !check_header + DIE\n> combination to reflect this.\n>\n> Helped-by: Jonathan Tan \n> Helped-by: Jeff King \n> Acked-by: Johannes Schindelin \n> Signed-off-by: Kyle J. McKay \n> ---\n>\n> Notes:\n> Please include this PATCH in 2.11.x maint\n>\n> mailinfo.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/mailinfo.c b/mailinfo.c\n> index 2fb3877e..a489d9d0 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -710,7 +710,8 @@ static void flush_inbody_header_accum(struct mailinfo *mi)\n> {\n> \tif (!mi->inbody_header_accum.len)\n> \t\treturn;\n> -\tassert(check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0));\n> +\tif (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0))\n> +\t\tdie(\"BUG: inbody_header_accum, if not empty, must always contain a valid in-body header\");\n> \tstrbuf_reset(&mi->inbody_header_accum);\n> }\n> \n> ---\n"},{"id":"308124","messageId":"735f6ca98151fc081ea871016afc4b6@58437222ff6db9ee7cbe9d1a5a1ad4e","threadId":"44727","inReplyTo":"xmqqbmw7mrg4.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v3] mailinfo.c: move side-effects outside of assert","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2016-12-19T23:54:41Z","receivedAt":"2016-12-19T23:55:43Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Dec 19, 2016, at 15:26, Junio C Hamano wrote:\n\n> \"Kyle J. McKay\" writes:\n>\n>>> OK. So we do not expect it to fail, but we still do want the side\n>>> effect of that function (i.e. accmulation into the field).\n>>>\n>>> Somebody care to send a final \"agreed-upon\" version?\n>>\n>> Yup, here it is:\n>\n> Thanks.\n\nWhoops. there's an extra paragraph in the commit description that I\nmeant to remove and, of course, I didn't notice it until I sent the\ncopy to the list. :(\n\nI don't think a \"fixup\" or \"squash\" can replace a description, right?\n\nSo here's a replacement patch with the correct description with the\ndeleted paragrah:\n\n-- >8 --\n\nSince 6b4b013f18 (mailinfo: handle in-body header continuations,\n2016-09-20, v2.11.0) mailinfo.c has contained new code with an\nassert of the form:\n\n\tassert(call_a_function(...))\n\nThe function in question, check_header, has side effects. This\nmeans that when NDEBUG is defined during a release build the\nfunction call is omitted entirely, the side effects do not\ntake place and tests (fortunately) start failing.\n\nSince the only time that mi->inbody_header_accum is appended to is\nin check_inbody_header, and appending onto a blank\nmi->inbody_header_accum always happens when is_inbody_header is\ntrue, this guarantees a prefix that causes check_header to always\nreturn true.\n\nTherefore replace the assert with an if !check_header + DIE\ncombination to reflect this.\n\nHelped-by: Jonathan Tan \nHelped-by: Jeff King \nAcked-by: Johannes Schindelin \nSigned-off-by: Kyle J. McKay \n---\n\nNotes:\n Please include this PATCH in 2.11.x maint\n\n mailinfo.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 2fb3877e..a489d9d0 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -710,7 +710,8 @@ static void flush_inbody_header_accum(struct mailinfo *mi)\n {\n \tif (!mi->inbody_header_accum.len)\n \t\treturn;\n-\tassert(check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0));\n+\tif (!check_header(mi, &mi->inbody_header_accum, mi->s_hdr_data, 0))\n+\t\tdie(\"BUG: inbody_header_accum, if not empty, must always contain a valid in-body header\");\n \tstrbuf_reset(&mi->inbody_header_accum);\n }\n \n---\n"},{"id":"308142","messageId":"alpine.DEB.2.20.1612201511480.54750@virtualbox","threadId":"44727","inReplyTo":"A916CED6-C49D-41D8-A7EE-A5FEDA641F4A@gmail.com","subject":"Re: [PATCH] mailinfo.c: move side-effects outside of assert","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-12-20T14:12:35Z","receivedAt":"2016-12-20T14:13:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Kyle,\n\nOn Mon, 19 Dec 2016, Kyle J. McKay wrote:\n\n> On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:\n> \n> >ACK. I noticed this problem (and fixed it independently as a part of a\n> >huge patch series I did not get around to submit yet) while trying to\n> >get Git to build correctly with Visual C.\n> \n> Does this mean that Dscho and I are the only ones who add -DNDEBUG for\n> release builds? Or are we just the only ones who actually run the test\n> suite on such builds?\n\nIt seems you and I are for the moment the only ones bothering with running\nthe test suite on release builds.\n\nCiao,\nJohannes\n"},{"id":"308146","messageId":"20161220164526.qnwnmr7cvyycmw6a@sigill.intra.peff.net","threadId":"44727","inReplyTo":"alpine.DEB.2.20.1612201511480.54750@virtualbox","subject":"Re: [PATCH] mailinfo.c: move side-effects outside of assert","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-20T16:45:26Z","receivedAt":"2016-12-20T16:45:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 20, 2016 at 03:12:35PM +0100, Johannes Schindelin wrote:\n\n> > On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:\n> > \n> > >ACK. I noticed this problem (and fixed it independently as a part of a\n> > >huge patch series I did not get around to submit yet) while trying to\n> > >get Git to build correctly with Visual C.\n> > \n> > Does this mean that Dscho and I are the only ones who add -DNDEBUG for\n> > release builds? Or are we just the only ones who actually run the test\n> > suite on such builds?\n> \n> It seems you and I are for the moment the only ones bothering with running\n> the test suite on release builds.\n\nI wasn't aware anybody actually built with NDEBUG at all. You'd have to\nexplicitly ask for it via CFLAGS, so I assume most people don't.\nCertainly I never have when deploying to GitHub's cluster (let alone my\npersonal use), and I note that the Debian package also does not.\n\nSo from my perspective it is not so much \"do not bother with release\nbuilds\" as \"are release builds even a thing for git?\". One of the\nreasons I suggested switching the assert() to a die(\"BUG\") is that the\nlatter cannot be disabled. We generally seem to prefer those to assert()\nin our code-base (though there is certainly a mix). If the assertions\nare not expensive to compute, I think it is better to keep them in for\nall builds. I'd much rather get a report from a user that says \"I hit\nthis BUG\" than \"git segfaulted and I have no idea where\" (of course I\nprefer a backtrace even more, but that's not always an option).\n\nI do notice that we set NDEBUG for nedmalloc, though if I am reading the\nMakefile right, it is just for compiling those files. It looks like\nthere are a ton of asserts there that _are_ potentially expensive, so\nthat makes sense.\n\n-Peff\n"},{"id":"308192","messageId":"222ACFD4-ED9A-4B94-8BDD-3C70648A684B@gmail.com","threadId":"44727","inReplyTo":"20161220164526.qnwnmr7cvyycmw6a@sigill.intra.peff.net","subject":"Re: [PATCH] mailinfo.c: move side-effects outside of assert","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2016-12-21T05:54:15Z","receivedAt":"2016-12-21T05:54:25Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Dec 20, 2016, at 08:45, Jeff King wrote:\n\n> On Tue, Dec 20, 2016 at 03:12:35PM +0100, Johannes Schindelin wrote:\n>\n>>> On Dec 19, 2016, at 09:45, Johannes Schindelin wrote:\n>>>\n>>>> ACK. I noticed this problem (and fixed it independently as a part \n>>>> of a\n>>>> huge patch series I did not get around to submit yet) while \n>>>> trying to\n>>>> get Git to build correctly with Visual C.\n>>>\n>>> Does this mean that Dscho and I are the only ones who add -DNDEBUG \n>>> for\n>>> release builds? Or are we just the only ones who actually run the \n>>> test\n>>> suite on such builds?\n>>\n>> It seems you and I are for the moment the only ones bothering with \n>> running\n>> the test suite on release builds.\n>\n> I wasn't aware anybody actually built with NDEBUG at all. You'd have \n> to\n> explicitly ask for it via CFLAGS, so I assume most people don't.\n\nNot a good assumption. You know what happens when you assume[1], \nright? ;)\n\nI've been defining NDEBUG whenever I make a release build for quite \nsome time (not just for Git) in order to squeeze every last possible \ndrop of performance out of it.\n\n> Certainly I never have when deploying to GitHub's cluster (let alone \n> my\n> personal use), and I note that the Debian package also does not.\n\nYeah, I don't do it for my personal use because those are often not \nbased on a release tag so I want to see any assertion failures that \nmight happen and they're also not performance critical either.\n\n> So from my perspective it is not so much \"do not bother with release\n> builds\" as \"are release builds even a thing for git?\"\n\nThey should be if you're deploying Git in a performance critical \nenvironment.\n\n> One of the\n> reasons I suggested switching the assert() to a die(\"BUG\") is that the\n> latter cannot be disabled. We generally seem to prefer those to \n> assert()\n> in our code-base (though there is certainly a mix). If the assertions\n> are not expensive to compute, I think it is better to keep them in for\n> all builds. I'd much rather get a report from a user that says \"I hit\n> this BUG\" than \"git segfaulted and I have no idea where\" (of course I\n> prefer a backtrace even more, but that's not always an option).\n\nPerhaps Git should provide a \"verify\" macro. Works like \"assert\" \nexcept that it doesn't go away when NDEBUG is defined. Being Git- \nprovided it could also use Git's die function. Then Git could do a \nglobal replace of assert with verify and institute a no-assert policy.\n\n> I do notice that we set NDEBUG for nedmalloc, though if I am reading \n> the\n> Makefile right, it is just for compiling those files. It looks like\n> there are a ton of asserts there that _are_ potentially expensive, so\n> that makes sense.\n\nSo there's no way to get a non-release build of nedmalloc inside Git \nthen without hacking the Makefile? What if you need those assertions \nenabled? Maybe NDEBUG shouldn't be defined by default for any files.\n\n--Kyle\n\n[1] https://www.youtube.com/watch?v=KEP1acj29-Y\n"},{"id":"308198","messageId":"20161221155539.aykcmkuzqvq733ri@sigill.intra.peff.net","threadId":"44727","inReplyTo":"222ACFD4-ED9A-4B94-8BDD-3C70648A684B@gmail.com","subject":"Re: [PATCH] mailinfo.c: move side-effects outside of assert","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-21T15:55:39Z","receivedAt":"2016-12-21T15:55:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote:\n\n> > I wasn't aware anybody actually built with NDEBUG at all. You'd have to\n> > explicitly ask for it via CFLAGS, so I assume most people don't.\n> \n> Not a good assumption. You know what happens when you assume[1], right? ;)\n\nKind of. If it's a configuration that nobody[1] in the Git development\ncommunity intended to support or test, then isn't the person triggering\nit the one making assumptions?\n\nAt any rate, I agree that setting NDEBUG should not create a broken\nprogram, and some solution like your patch is a good idea here. I was\nmainly speaking to the \"do not bother\" comment. It is not that I do not\nbother to build with NDEBUG, it is that I think it is actively a bad\nidea.\n\n[1] Maybe I am alone in my surprise, and everybody working on Git is\n using assert() with the intention that it can be disabled. But if\n that were the case, I'd expect more push-back against \"die(BUG)\"\n which does not have this feature. I don't recall a single discussion\n to that effect, and searching for NDEBUG in the list archives turns\n up hardly any mentions.\n\n> I've been defining NDEBUG whenever I make a release build for quite some\n> time (not just for Git) in order to squeeze every last possible drop of\n> performance out of it.\n\nI think here you are getting into superstition. Is there any single\nassert() in Git that will actually have an impact on performance?\n\nI'd be more impressed if you could show some operation that is faster\nwhen built with NDEBUG than without. Running all of t/perf does not seem\nto show any difference, and looking at the asserts themselves, they're\nalmost all single-instruction compares in code that isn't performance\ncritical anyway.\n\n> > So from my perspective it is not so much \"do not bother with release\n> > builds\" as \"are release builds even a thing for git?\"\n> \n> They should be if you're deploying Git in a performance critical\n> environment.\n\nI hope my history of patches shows that I do care about deploying Git in\na performance critical environment. But I only care about performance\ntradeoffs that have a _measurable_ gain.\n\n> Perhaps Git should provide a \"verify\" macro. Works like \"assert\" except\n> that it doesn't go away when NDEBUG is defined. Being Git-provided it could\n> also use Git's die function. Then Git could do a global replace of assert\n> with verify and institute a no-assert policy.\n\nWhat would be the advantage of that over `if(...) die(\"BUG: ...\")`? It\ndoes not require you to write a reason in the die(), but I am not sure\nthat is a good thing.\n\n> > I do notice that we set NDEBUG for nedmalloc, though if I am reading the\n> > Makefile right, it is just for compiling those files. It looks like\n> > there are a ton of asserts there that _are_ potentially expensive, so\n> > that makes sense.\n> \n> So there's no way to get a non-release build of nedmalloc inside Git then\n> without hacking the Makefile? What if you need those assertions enabled?\n> Maybe NDEBUG shouldn't be defined by default for any files.\n\nAFAICT, yes. I'd leave it to people who actually build with nedmalloc to\ndecide whether it is worth caring about, and whether the asserts there\nhave a noticeable performance impact.\n\n-Peff\n"},{"id":"308233","messageId":"F5001DF2-20C2-4757-997F-9D40BD48E1D9@gmail.com","threadId":"44727","inReplyTo":"20161221155539.aykcmkuzqvq733ri@sigill.intra.peff.net","subject":"Re: [PATCH] mailinfo.c: move side-effects outside of assert","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2016-12-22T02:21:37Z","receivedAt":"2016-12-22T02:21:48Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Dec 21, 2016, at 07:55, Jeff King wrote:\n\n> On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote:\n>\n>>> I wasn't aware anybody actually built with NDEBUG at all. You'd \n>>> have to\n>>> explicitly ask for it via CFLAGS, so I assume most people don't.\n>>\n>> Not a good assumption. You know what happens when you assume[1], \n>> right? ;)\n>\n> Kind of. If it's a configuration that nobody[1] in the Git development\n> community intended to support or test, then isn't the person \n> triggering\n> it the one making assumptions?\n\nNo, I don't think so. NDEBUG is very clearly specified in POSIX [1].\n\nIf NDEBUG is defined then \"assert(...)\" disappears (and in a nice way \nso as not to precipitate \"unused variable\" warnings). \"N\" being \"No\" \nor \"Not\" or \"Negated\" or \"bar over the top\" + \"DEBUG\" meaning Not \nDEBUG. So the code that goes away when NDEBUG is defined is clearly \ndebug code.\n\nConsidering the wide deployment and use of Git at this point I think \nrather the opposite to be true that \"Git does Not require DEBUGging \ncode to be enabled for everyday use.\" The alternative that it does \nsuggests it's not ready for prime time and quite clearly that's not \nthe case.\n\n>> I've been defining NDEBUG whenever I make a release build for quite \n>> some\n>> time (not just for Git) in order to squeeze every last possible \n>> drop of\n>> performance out of it.\n>\n> I think here you are getting into superstition. Is there any single\n> assert() in Git that will actually have an impact on performance?\n\nYou have suggested there is and that Git is enabling NDEBUG for \nexactly that reason -- to increase performance:\n\n>> On Dec 20, 2016, at 08:45, Jeff King wrote:\n>>\n>>> I do notice that we set NDEBUG for nedmalloc, though if I am \n>>> reading the\n>>> Makefile right, it is just for compiling those files. It looks like\n>>> there are a ton of asserts there that _are_ potentially expensive\n\n\n>> Perhaps Git should provide a \"verify\" macro. Works like \"assert\" \n>> except\n>> that it doesn't go away when NDEBUG is defined. Being Git-provided \n>> it could\n>> also use Git's die function. Then Git could do a global replace of \n>> assert\n>> with verify and institute a no-assert policy.\n>\n> What would be the advantage of that over `if(...) die(\"BUG: ...\")`? It\n> does not require you to write a reason in the die(), but I am not sure\n> that is a good thing.\n\nYou have stated that you believe the current \"assert\" calls in Git \n(excluding nedmalloc) should not magically disappear when NDEBUG is \ndefined. So precluding a more labor intensive approach where all \ncurrently existing \"assert(...)\" calls are replaced with an \"if (!...) \ndie(...)\" combination, providing a \"verify\" macro is a quick way to \nmake that happen. Consider this, was the value that Jonathan provided \nfor the \"die\" string immediately obvious to you? It sure wasn't to \nme. That means that whoever does the \"assert(...)\" -> \"if(!...)die\" \nswap out may need to be intimately familiar with that particular piece \nof code or the result will be no better than using a \"verify\" macro.\n\nI'm just trying to find a quick and easy way to accommodate your \nwishes without redefining the semantics of NDEBUG. ;)\n\n--Kyle\n\n[1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/assert.h.html\n"},{"id":"308234","messageId":"20161222033418.dmslmuhq7mqhmkwq@sigill.intra.peff.net","threadId":"44727","inReplyTo":"F5001DF2-20C2-4757-997F-9D40BD48E1D9@gmail.com","subject":"Re: [PATCH] mailinfo.c: move side-effects outside of assert","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-22T03:34:19Z","receivedAt":"2016-12-22T03:34:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 21, 2016 at 06:21:37PM -0800, Kyle J. McKay wrote:\n\n> > Kind of. If it's a configuration that nobody[1] in the Git development\n> > community intended to support or test, then isn't the person triggering\n> > it the one making assumptions?\n> \n> No, I don't think so. NDEBUG is very clearly specified in POSIX [1].\n\nI know what NDEBUG is, and I know it is widely used in other projects.\nMy claim is only that I do not think that the use of NDEBUG was\ngenerally considered when people wrote assert() in git. That is\ncertainly true of me. I don't know about other developers.\n\n> > I think here you are getting into superstition. Is there any single\n> > assert() in Git that will actually have an impact on performance?\n> \n> You have suggested there is and that Git is enabling NDEBUG for exactly that\n> reason -- to increase performance:\n\nSorry if I wasn't clear, but I meant in Git itself, not in the nedmalloc\ncompat code. I think it's worth considering them separately because the\nlatter is not part of most Git builds in the first place, and certainly\nit is written in a different style, and may have different assumptions\nabout how people might build it.\n\nI do not know the nedmalloc code well at all. I gave a brief read of the\nresults of \"git grep 'assert('\" and it looked like some of its\nassertions are more involved than simple comparisons. So I guessed that\nperhaps the reason that NDEBUG was added there when the code was\nimported is that there is a performance difference there. But that's\njust a guess. It may or may not be borne out by measurements.\n\nI'd be surprised if any of the assertions in the rest of git have a\nnoticeable impact (and I could not detect one by running t/perf).\n\n> > What would be the advantage of that over `if(...) die(\"BUG: ...\")`? It\n> > does not require you to write a reason in the die(), but I am not sure\n> > that is a good thing.\n> \n> You have stated that you believe the current \"assert\" calls in Git\n> (excluding nedmalloc) should not magically disappear when NDEBUG is defined.\n\nWell, no, I mostly just said that I do not think there is any point in\ndefining NDEBUG in the first place, as there is little or no benefit to\nremoving those asserts from the built product.\n\n> So precluding a more labor intensive approach where all currently existing\n> \"assert(...)\" calls are replaced with an \"if (!...) die(...)\" combination,\n> providing a \"verify\" macro is a quick way to make that happen. Consider\n> this, was the value that Jonathan provided for the \"die\" string immediately\n> obvious to you? It sure wasn't to me. That means that whoever does the\n> \"assert(...)\" -> \"if(!...)die\" swap out may need to be intimately familiar\n> with that particular piece of code or the result will be no better than\n> using a \"verify\" macro.\n\nSure, if you want to mass-convert them, doing so with a macro similar to\nassert is the simplest way. I don't think we are in a huge hurry to do\nthat conversion though. I'm not complaining that NDEBUG works as\nadvertised by disabling asserts. I'm just claiming that it's largely\npointless in our code base, and I'd consider die(\"BUG\") to be our\n\"usual\" style. If any change is to be made, it would be to suggest\npeople prefer die(\"BUG\") as a style guideline for future patches. But I\nhaven't seen anybody agree (or disagree) with the notion, so I'd\nhesitate to impose my style suggestion without more discussion in that\narea.\n\nIf we _did_ accept that as a style guideline, then we could move over\nexisting assert() calls over time (either as janitorial projects, or as\npeople touch the related code). But there's not a pressing need to do it\nquickly.\n\n-Peff\n"},{"id":"308235","messageId":"99C4A905-D66B-4609-9E55-06F9BC301C74@gmail.com","threadId":"44727","inReplyTo":"F5001DF2-20C2-4757-997F-9D40BD48E1D9@gmail.com","subject":"Re: [PATCH] mailinfo.c: move side-effects outside of assert","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2016-12-22T03:53:15Z","receivedAt":"2016-12-22T03:55:02Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Dec 21, 2016, at 18:21, Kyle J. McKay wrote:\n\n> On Dec 21, 2016, at 07:55, Jeff King wrote:\n>\n>> On Tue, Dec 20, 2016 at 09:54:15PM -0800, Kyle J. McKay wrote:\n>>\n>>>> I wasn't aware anybody actually built with NDEBUG at all. You'd \n>>>> have to\n>>>> explicitly ask for it via CFLAGS, so I assume most people don't.\n>>>\n>>> Not a good assumption. You know what happens when you assume[1], \n>>> right? ;)\n>>\n>> Kind of. If it's a configuration that nobody[1] in the Git \n>> development\n>> community intended to support or test, then isn't the person \n>> triggering\n>> it the one making assumptions?\n>\n> No, I don't think so. NDEBUG is very clearly specified in POSIX [1].\n>\n> If NDEBUG is defined then \"assert(...)\" disappears (and in a nice \n> way so as not to precipitate \"unused variable\" warnings). \"N\" being \n> \"No\" or \"Not\" or \"Negated\" or \"bar over the top\" + \"DEBUG\" meaning \n> Not DEBUG. So the code that goes away when NDEBUG is defined is \n> clearly debug code.\n\nI think there is a useful distinction here that I make that's worth \nsharing. Perhaps it's splitting hairs, but I categorize this \"extra\" \ncode that we've been discussing (\"assert(...)\" or \"if (!...) die(...)\" \nor \"verify(...)\" into two groups:\n\n\n1) DEBUG code\n\nThis is code that developers use when creating new features. Or \nhelpful code that's needed when stepping through a program with the \ndebugger to debug a problem. Or even code that's only used by some \nkind of external \"test\". It may be expensive, it may do things that \nshould never be done in a build for wider consumption (such as write \ninformation to special log files, write special syslog messages \netc.). Often this code is used in combination with a \"-g\" debug \nsymbols build and possibly even a \"-O0\" or \"-O1\" option.\n\nCode like this has no place in a release executable meant for general \nuse by an end user.\n\n2) DIAGNOSTIC code\n\nThis is near zero overhead code that is intended to be left in a \nrelease build meant for general use and normally sits there not doing \nanything and NOT leaching any performance out of the build either. \nIts sole purpose in life is to provide a trail of \"bread crumbs\" if \nthe executable goes ***BOOM***. These \"bread crumbs\" should be just \nenough when combined with feedback from the unfortunate user who \nexperienced the meltdown to re-create the issue in a real DEBUG build \nand find and fix the problem.\n\n\nIt seems to me what you are saying is that Git's \"assert\" calls are \nDIAGNOSTIC and therefore belong in a release build -- well, except for \nthe nedmalloc \"assert\" calls which do not.\n\nWhat I'm saying is if they are diagnostic and not debug (and I'm not \narguing one way or the other, but you've already indicated they are \nnear zero overhead which suggests they are indeed diagnostic in \nnature), then they do not belong inside an \"assert\" which can be \ndisabled with \"NDEBUG\". I'm arguing that \"assert\" is not intended for \ndiagnostic code, but only debug code as used by nedmalloc. Having Git \ntreat \"NDEBUG\" one way -- \"no, no, do NOT define NDEBUG because that \ndisables Git diagnostics and I promise you there's no performance \npenalty\" -- versus nedmalloc -- \"yes, yes please DO define NDEBUG \nunless you really need our slow debugging code to be present for \ndebugging purposes\" -- just creates needless unnecessary confusion.\n\n--Kyle\n"},{"id":"308236","messageId":"20161222035923.chgdv7pcbzevihhm@sigill.intra.peff.net","threadId":"44727","inReplyTo":"99C4A905-D66B-4609-9E55-06F9BC301C74@gmail.com","subject":"Re: [PATCH] mailinfo.c: move side-effects outside of assert","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-22T03:59:23Z","receivedAt":"2016-12-22T03:59:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 21, 2016 at 07:53:15PM -0800, Kyle J. McKay wrote:\n\n> It seems to me what you are saying is that Git's \"assert\" calls are\n> DIAGNOSTIC and therefore belong in a release build -- well, except for the\n> nedmalloc \"assert\" calls which do not.\n\nYes, I think that is a good way of thinking about it (modulo that I\nreally can't say one way or the other about nedmalloc's uses).\n\nThere _are_ some DEBUG-type things in Git that are protected by #ifdefs\nthat default to \"off\" (grep for DIFF_DEBUG, for instance). I'm actually\nof the opinion that debugging code like that should be in all builds and\ntriggerable at run-time, provided it carries no significant performance\npenalty when the run-time switch is not enabled. But I do agree that's a\ntotally separate question than from your DEBUG/DIAGNOSTIC distinction.\n\n-Peff\n"},{"id":"308252","messageId":"xmqq7f6rhmnu.fsf@gitster.mtv.corp.google.com","threadId":"44727","inReplyTo":"20161222033418.dmslmuhq7mqhmkwq@sigill.intra.peff.net","subject":"Re: [PATCH] mailinfo.c: move side-effects outside of assert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-22T17:57:25Z","receivedAt":"2016-12-22T17:57:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Well, no, I mostly just said that I do not think there is any point in\n> defining NDEBUG in the first place, as there is little or no benefit to\n> removing those asserts from the built product.\n> ...\n> Sure, if you want to mass-convert them, doing so with a macro similar to\n> assert is the simplest way. I don't think we are in a huge hurry to do\n> that conversion though. I'm not complaining that NDEBUG works as\n> advertised by disabling asserts. I'm just claiming that it's largely\n> pointless in our code base, and I'd consider die(\"BUG\") to be our\n> \"usual\" style. \n\nI agree with all of the above. Given the way how our own code uses\nassert(), there is little point removing them and turning them over\ntime into \"if (...) die(BUG)\" would probably be better.\n\nBorrowed code like nedmalloc may be a different story, but as you\nsaid in a separate message in this thread, I think we are better off\nleaving that to those who care about that piece of code.\n"}]}