{"thread":{"id":"20943","subject":"[PATCH 2/2] remove NORETURN from function pointers","startedAt":"2009-09-14T10:16:10Z","lastAt":"2009-09-30T18:10:32Z","messageCount":17,"participants":["Erik Faye-Lund","Jeff King","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"123138","messageId":"1252923370-5768-2-git-send-email-kusmabite@gmail.com","threadId":"20943","inReplyTo":"1252923370-5768-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH 2/2] remove NORETURN from function pointers","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-09-14T10:16:10Z","receivedAt":"2009-09-14T10:16:10Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"From: Erik Faye-Lund \n\nSome compilers (including at least MSVC and ARM RVDS) supports\nNORETURN on function declarations, but not on function pointers.\n\nThis patch makes it possible to define NORETURN for these compilers.\n\nSigned-off-by: Erik Faye-Lund \n---\n\n__attribute__((noreturn)) is, according to the GCC documentation, about\ntwo things: code generation (performance, really) and warnings.\n\nOn the warnings-side, we need to keep the code warning-free for\ncompilers who doesn't support noreturn anyway, so hiding potential\nwarnings through this mechanism is probably not a good idea in the\nfirst place.\n\nWe still want the performance-side of it, though. However, the only\nplace this really makes a difference is to die and it's variants, since\nthey can potentially be called many times (or so it seems from the\ncompiler's point of view without a noreturn declaration).\n\nThe function pointers are only called once we're already exiting, and\nthey have only one potential call-site.\n\nI hope this all makes sense ;)\n\n git-compat-util.h | 2 +-\n usage.c | 6 +++---\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 5876d91..15fe08e 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -183,7 +183,7 @@ extern NORETURN void die_errno(const char *err, ...) __attribute__((format (prin\n extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n \n-extern void set_die_routine(void (*routine)(const char *err, va_list params) NORETURN);\n+extern void set_die_routine(void (*routine)(const char *err, va_list params));\n \n extern int prefixcmp(const char *str, const char *prefix);\n extern time_t tm_to_time_t(const struct tm *tm);\ndiff --git a/usage.c b/usage.c\nindex b6aea45..18d7f43 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -36,12 +36,12 @@ static void warn_builtin(const char *warn, va_list params)\n \n /* If we are in a dlopen()ed .so write to a global variable would segfault\n * (ugh), so keep things static. */\n-static void (*usage_routine)(const char *err) NORETURN = usage_builtin;\n-static void (*die_routine)(const char *err, va_list params) NORETURN = die_builtin;\n+static void (*usage_routine)(const char *err) = usage_builtin;\n+static void (*die_routine)(const char *err, va_list params) = die_builtin;\n static void (*error_routine)(const char *err, va_list params) = error_builtin;\n static void (*warn_routine)(const char *err, va_list params) = warn_builtin;\n \n-void set_die_routine(void (*routine)(const char *err, va_list params) NORETURN)\n+void set_die_routine(void (*routine)(const char *err, va_list params))\n {\n \tdie_routine = routine;\n }\n-- \n1.6.4.msysgit.0.16.gd92d4.dirty\n"},{"id":"123139","messageId":"20090914105750.GB9216@sigill.intra.peff.net","threadId":"20943","inReplyTo":"1252923370-5768-2-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH 2/2] remove NORETURN from function pointers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-09-14T10:57:50Z","receivedAt":"2009-09-14T10:57:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 14, 2009 at 12:16:10PM +0200, Erik Faye-Lund wrote:\n\n> From: Erik Faye-Lund \n> \n> Some compilers (including at least MSVC and ARM RVDS) supports\n> NORETURN on function declarations, but not on function pointers.\n> \n> This patch makes it possible to define NORETURN for these compilers.\n> \n> Signed-off-by: Erik Faye-Lund \n\nI didn't see a patch 1/2, so maybe it impacts this in some way, but by\nitself, I don't think this patch is a good idea. See below.\n\n> ---\n> \n> __attribute__((noreturn)) is, according to the GCC documentation, about\n> two things: code generation (performance, really) and warnings.\n> \n> On the warnings-side, we need to keep the code warning-free for\n> compilers who doesn't support noreturn anyway, so hiding potential\n> warnings through this mechanism is probably not a good idea in the\n> first place.\n\n[Your justification text would almost certainly be useful as part of the\ncommit message itself, and should go there.]\n\nUnfortunately, this patch _introduces_ warnings when running with gcc,\nas it now thinks those function pointers return (which means it thinks\ndie() returns). So simply removing the NORETURN is not a good idea.\n\nIf I understand you correctly, the problem is that there are actually\nthree classes of compilers:\n\n 1. Ones which understand some NORETURN syntax for both functions and\n function pointers, and correctly trace returns through both (e.g.,\n gcc).\n\n 2. Ones which understand some NORETURN syntax for just functions, and\n complain about it on function pointers. We currently turn off\n NORETURN for these compilers (from your commit message, MSVC,\n for example).\n\n 3. Ones which have no concept of NORETURN at all.\n\nI think the right solution to turn on NORETURN for (2) is to split it\ninto two cases: NORETURN and NORETURN_PTR. Gcc platforms can define both\nidentically, platforms under (2) above can define NORETURN_PTR as empty,\nand (3) will keep both off.\n\nI do have to wonder, though. What do compilers that fall under (2) do\nwith calls to function pointers from NORETURN functions? Do they assume\nthey don't return, or that they do? Or do they not check that NORETURN\nfunctions actually don't return?\n\nI.e., what does this program do under MSVC:\n\n#include \nvoid (*exit_fun)(int) = exit;\nstatic void die(void) __attribute__((__noreturn__));\nstatic void die(void) { exit_fun(1); }\nint main(void) { die(); }\n\nIn gcc, it rightly complains:\n\n foo.c: In function ‘die’:\n foo.c:4: warning: ‘noreturn’ function does return\n\n-Peff\n"},{"id":"123142","messageId":"40aa078e0909140440x2e189957uf66f36ff29bef302@mail.gmail.com","threadId":"20943","inReplyTo":"20090914105750.GB9216@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] remove NORETURN from function pointers","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-09-14T11:40:02Z","receivedAt":"2009-09-14T11:40:02Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Sep 14, 2009 at 12:57 PM, Jeff King wrote:\n>\n> I didn't see a patch 1/2, so maybe it impacts this in some way, but by\n> itself, I don't think this patch is a good idea. See below.\n\nThat's odd. It's listed in my git-folder on GMail as sent to the\nmailing-list, but I can't find it in any of the list-archives. They\nwere both sent through the same instance of \"git send-email\". I guess\nI'll just re-send it. It shouldn't affect this patch directly, though.\n\nThe patch basically moved the NORETURN before the function-name, as\nthis is a placement where more compilers supports\ndeclaration-specifications.\n\n>> ---\n>>\n>> __attribute__((noreturn)) is, according to the GCC documentation, about\n>> two things: code generation (performance, really) and warnings.\n>>\n>> On the warnings-side, we need to keep the code warning-free for\n>> compilers who doesn't support noreturn anyway, so hiding potential\n>> warnings through this mechanism is probably not a good idea in the\n>> first place.\n>\n> [Your justification text would almost certainly be useful as part of the\n> commit message itself, and should go there.]\n\nOK, I'll include it in the next round.\n\n> Unfortunately, this patch _introduces_ warnings when running with gcc,\n> as it now thinks those function pointers return (which means it thinks\n> die() returns). So simply removing the NORETURN is not a good idea.\n\nYeah, this is unacceptable. I can't believe I missed this - sorry about that!\n\n> If I understand you correctly, the problem is that there are actually\n> three classes of compilers:\n>\n>  1. Ones which understand some NORETURN syntax for both functions and\n>     function pointers, and correctly trace returns through both (e.g.,\n>     gcc).\n>\n>  2. Ones which understand some NORETURN syntax for just functions, and\n>     complain about it on function pointers. We currently turn off\n>     NORETURN for these compilers (from your commit message, MSVC,\n>     for example).\n>\n>  3. Ones which have no concept of NORETURN at all.\n>\n> I think the right solution to turn on NORETURN for (2) is to split it\n> into two cases: NORETURN and NORETURN_PTR. Gcc platforms can define both\n> identically, platforms under (2) above can define NORETURN_PTR as empty,\n> and (3) will keep both off.\n\nYeah, this could work. I initially suggested doing this, but Junio\nsuggested removing NORETURN all together. I didn't think that was a\ngood idea for die() etc, thus this patch.\n\n> I do have to wonder, though. What do compilers that fall under (2) do\n> with calls to function pointers from NORETURN functions? Do they assume\n> they don't return, or that they do? Or do they not check that NORETURN\n> functions actually don't return?\n>\n> I.e., what does this program do under MSVC:\n>\n> #include \n> void (*exit_fun)(int) = exit;\n> static void die(void) __attribute__((__noreturn__));\n> static void die(void) { exit_fun(1); }\n> int main(void) { die(); }\n\nWell, it fails to compile ;)\n\nIf your change it around this way (which is basically what 1/2 + a\nseparate patch that is cooking in msysgit for a litte while longer is\nsupposed to do), it does compiles without warnings even on the highest\nwarning level:\n\n-static void die(void) __attribute__((__noreturn__));\n+static void __declspec(noreturn) die(void);\n\n> In gcc, it rightly complains:\n>\n>  foo.c: In function ‘die’:\n>  foo.c:4: warning: ‘noreturn’ function does return\n\nYeah. So we need a portable (enough) way of showing it that it does\ndie. How about abort()?\n\n-static void die(void) { exit_fun(1); }\n+static void die(void) { exit_fun(1); abort(); }\n\nAdding abort() makes the warning go away here, at least. And reaching\nthis point is an error anyway, it means that one of the functions\npassed to set_die_routine() does not hold up it's guarantees. Your\nsuggestion (double defines) would make this a compile-time warning\ninstead of a run-time error, which I find much more elegant. However,\nit's questionable how much this means in reality - there's only two\ncall-sites for set_die_routine() ATM. Do we expect it to grow a lot?\n\n-- \nErik \"kusma\" Faye-Lund\nkusmabite@gmail.com\n(+47) 986 59 656\n"},{"id":"123144","messageId":"40aa078e0909140456l47cfce92yc44262c96b59bf2d@mail.gmail.com","threadId":"20943","inReplyTo":"40aa078e0909140440x2e189957uf66f36ff29bef302@mail.gmail.com","subject":"Re: [PATCH 2/2] remove NORETURN from function pointers","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-09-14T11:56:29Z","receivedAt":"2009-09-14T11:56:29Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Sep 14, 2009 at 1:40 PM, Erik Faye-Lund\n wrote:\n> On Mon, Sep 14, 2009 at 12:57 PM, Jeff King wrote:\n>>\n>> I didn't see a patch 1/2, so maybe it impacts this in some way, but by\n>> itself, I don't think this patch is a good idea. See below.\n>\n> That's odd. It's listed in my git-folder on GMail as sent to the\n> mailing-list, but I can't find it in any of the list-archives. They\n> were both sent through the same instance of \"git send-email\". I guess\n> I'll just re-send it.\n\nOK, I think I figured out why: For some reason, the From-field of the\nmail had gotten changed from kus...e@gmail.com to\nkus...e@googlemail.com, and that's not the address I'm subsribed to\nthis list as. I hope whoever manages the list is able to remove my\nduplicates from the moderation-queue or something ;)\n\n-- \nErik \"kusma\" Faye-Lund\nkusmabite@gmail.com\n(+47) 986 59 656\n"},{"id":"123145","messageId":"20090914120311.GA17172@sigill.intra.peff.net","threadId":"20943","inReplyTo":"40aa078e0909140440x2e189957uf66f36ff29bef302@mail.gmail.com","subject":"Re: [PATCH 2/2] remove NORETURN from function pointers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-09-14T12:03:11Z","receivedAt":"2009-09-14T12:03:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 14, 2009 at 01:40:02PM +0200, Erik Faye-Lund wrote:\n\n> > I didn't see a patch 1/2, so maybe it impacts this in some way, but by\n> > itself, I don't think this patch is a good idea. See below.\n> \n> That's odd. It's listed in my git-folder on GMail as sent to the\n> mailing-list, but I can't find it in any of the list-archives. They\n> were both sent through the same instance of \"git send-email\". I guess\n> I'll just re-send it. It shouldn't affect this patch directly, though.\n\nPossibly it got swallowed by vger's list filter. The taboo list is here:\n\n http://vger.kernel.org/majordomo-taboos.txt\n\n> > I think the right solution to turn on NORETURN for (2) is to split it\n> > into two cases: NORETURN and NORETURN_PTR. Gcc platforms can define both\n> > identically, platforms under (2) above can define NORETURN_PTR as empty,\n> > and (3) will keep both off.\n> \n> Yeah, this could work. I initially suggested doing this, but Junio\n> suggested removing NORETURN all together. I didn't think that was a\n> good idea for die() etc, thus this patch.\n\nDoing it this way would keep warnings on compilers that could use them.\nI am generally of the opinion that since most development happens on\ngcc, it is \"good enough\" to let gcc warnings help us find broken code,\nand then those fixes will be available to users of less-capable\ncompilers. And we don't have to bend over backwards in the code with\nlittle hacks to trick all compilers into not issuing a warning (like\ncalling abort() after something we already know is going to exit).\n\nThe downside of that attitude is that code that is not exercised by gcc\nbuilds does not get the benefit. And in the case of MSVC, there is\nprobably quite a bit of code in #ifdef's that gcc will never even parse.\nSo maybe a hack like abort() is worthwhile in this case.\n\n> > #include \n> > void (*exit_fun)(int) = exit;\n> > static void die(void) __attribute__((__noreturn__));\n> > static void die(void) { exit_fun(1); }\n> > int main(void) { die(); }\n> \n> Well, it fails to compile ;)\n> \n> If your change it around this way (which is basically what 1/2 + a\n> separate patch that is cooking in msysgit for a litte while longer is\n> supposed to do), it does compiles without warnings even on the highest\n> warning level:\n> \n> -static void die(void) __attribute__((__noreturn__));\n> +static void __declspec(noreturn) die(void);\n\nHmm. So either it doesn't bother checking that noreturn functions don't\nreturn, or it assumes that a function pointer may exit. Interesting,\nbut I guess it doesn't change the main point too much: it's not as\nstrict as gcc's checking.\n\n> Yeah. So we need a portable (enough) way of showing it that it does\n> die. How about abort()?\n> \n> -static void die(void) { exit_fun(1); }\n> +static void die(void) { exit_fun(1); abort(); }\n> \n> Adding abort() makes the warning go away here, at least. And reaching\n> this point is an error anyway, it means that one of the functions\n> passed to set_die_routine() does not hold up it's guarantees. Your\n> suggestion (double defines) would make this a compile-time warning\n> instead of a run-time error, which I find much more elegant. However,\n> it's questionable how much this means in reality - there's only two\n> call-sites for set_die_routine() ATM. Do we expect it to grow a lot?\n\nNo, I don't think we expect it to grow. Mostly this is about documenting\nour assumptions so that gcc can do the right thing in making die() a\nnoreturn function, which is what we actually care about. We would notice\nvery quickly, I think, if a die() handler did not actually exit.\n\nI think I am fine doing it either way. The NORETURN_PTR thing is a bit\nmore elegant to me, but that is maybe just my gcc snobiness. We\nshouldn't have to change our code to accomodate MSVC's crappy noreturn\nhandling. ;)\n\n-Peff\n"},{"id":"123146","messageId":"20090914120359.GB17172@sigill.intra.peff.net","threadId":"20943","inReplyTo":"40aa078e0909140456l47cfce92yc44262c96b59bf2d@mail.gmail.com","subject":"Re: [PATCH 2/2] remove NORETURN from function pointers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-09-14T12:04:00Z","receivedAt":"2009-09-14T12:04:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 14, 2009 at 01:56:29PM +0200, Erik Faye-Lund wrote:\n\n> > That's odd. It's listed in my git-folder on GMail as sent to the\n> > mailing-list, but I can't find it in any of the list-archives. They\n> > were both sent through the same instance of \"git send-email\". I guess\n> > I'll just re-send it.\n> \n> OK, I think I figured out why: For some reason, the From-field of the\n> mail had gotten changed from kus...e@gmail.com to\n> kus...e@googlemail.com, and that's not the address I'm subsribed to\n> this list as. I hope whoever manages the list is able to remove my\n> duplicates from the moderation-queue or something ;)\n\nI doubt that is it; you don't have to be subscribed to post.\n\n-Peff\n"},{"id":"123147","messageId":"40aa078e0909140532q693a7f9qc3d9b1d354cac356@mail.gmail.com","threadId":"20943","inReplyTo":"20090914120311.GA17172@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] remove NORETURN from function pointers","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-09-14T12:32:38Z","receivedAt":"2009-09-14T12:32:38Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Compiling the following code gives a warning about unreachable code,\nso it's clear that msvc doesn't simply ignore the directive. I'm not\nsaying that anyone suggested otherwise, I just wanted to know for\nsure.\n\n#include \n#include \nvoid (*exit_fun)(int) = exit;\nvoid __declspec(noreturn) die(void);\nvoid die(void) { exit_fun(1); }\nint main(void) { printf(\"hello!\\n\"); die(); printf(\"world!\\n\"); }\n\nOn Mon, Sep 14, 2009 at 2:03 PM, Jeff King wrote:\n> I think I am fine doing it either way. The NORETURN_PTR thing is a bit\n> more elegant to me, but that is maybe just my gcc snobiness. We\n> shouldn't have to change our code to accomodate MSVC's crappy noreturn\n> handling. ;)\n\nFirst of all, MSVC is not the only compiler that behaves this way. In\nfact, GCC the only compiler I've found that behaves this way (but I\nmust admit, I only tested 4 different compilers, one of which (Comeau)\ndoes not support noreturn at all AFAICT). That behavior might be\ncrappy, but it's not \"MSVC's crappy noreturn handling\" - it's\n\"non-GCC's crappy noreturn handling\" :P\n\nThe arguments against each solution I see are these:\n- abort() gives a run-time error instead of a compile-time warning, so\nbreakage is trickier to detect (on GCC, which seems to be the target\ncompiler for the vast majority of git-developers).\n- NORETURN_PTR might be bit big of a hammer for a small problem, as it\n\"pollutes\" the whole git source-tree instead of just usage.c.\n\nAnyway, I don't care much what solution we pick. Either should work,\nand if someone has strong preference, I'm OK with it.\n\n-- \nErik \"kusma\" Faye-Lund\nkusmabite@gmail.com\n(+47) 986 59 656\n"},{"id":"123148","messageId":"20090914124428.GA9394@coredump.intra.peff.net","threadId":"20943","inReplyTo":"40aa078e0909140532q693a7f9qc3d9b1d354cac356@mail.gmail.com","subject":"Re: [PATCH 2/2] remove NORETURN from function pointers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-09-14T12:44:28Z","receivedAt":"2009-09-14T12:44:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 14, 2009 at 02:32:38PM +0200, Erik Faye-Lund wrote:\n\n> Compiling the following code gives a warning about unreachable code,\n> so it's clear that msvc doesn't simply ignore the directive. I'm not\n> saying that anyone suggested otherwise, I just wanted to know for\n> sure.\n> \n> #include \n> #include \n> void (*exit_fun)(int) = exit;\n> void __declspec(noreturn) die(void);\n> void die(void) { exit_fun(1); }\n> int main(void) { printf(\"hello!\\n\"); die(); printf(\"world!\\n\"); }\n\nRight. What I'm guessing is that it sees the noreturn on die(), but then\ndoesn't actually look inside die to confirm that the noreturn is upheld.\nYou could test that with:\n\n#include \nvoid __declspec(noreturn) die(void);\nvoid die(void) { }\nint main(void) { die(); }\n\nIf it doesn't complain, then I am right. If it does, then it is\nsomething magic with the function pointer (presumably it decides that\nthe function pointer could exit, so it stops the analysis and gives your\ncode the benefit of the doubt).\n\n> First of all, MSVC is not the only compiler that behaves this way. In\n> fact, GCC the only compiler I've found that behaves this way (but I\n> must admit, I only tested 4 different compilers, one of which (Comeau)\n> does not support noreturn at all AFAICT). That behavior might be\n> crappy, but it's not \"MSVC's crappy noreturn handling\" - it's\n> \"non-GCC's crappy noreturn handling\" :P\n\nWell, OK, I'll accept that. ;)\n\n> The arguments against each solution I see are these:\n> - abort() gives a run-time error instead of a compile-time warning, so\n> breakage is trickier to detect (on GCC, which seems to be the target\n> compiler for the vast majority of git-developers).\n> - NORETURN_PTR might be bit big of a hammer for a small problem, as it\n> \"pollutes\" the whole git source-tree instead of just usage.c.\n\nI don't know that NORETURN_PTR pollutes the whole source-tree. At least\nno more than NORETURN does. The only functions that will need it are\nthese two function pointers.\n\nBut I think your analysis is generally correct. It's not going to make a\nbig difference which is chosen.\n\n-Peff\n"},{"id":"123150","messageId":"40aa078e0909140556j401085ccmdf43521b8b839e8e@mail.gmail.com","threadId":"20943","inReplyTo":"20090914124428.GA9394@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] remove NORETURN from function pointers","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-09-14T12:56:20Z","receivedAt":"2009-09-14T12:56:20Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Sep 14, 2009 at 2:44 PM, Jeff King wrote:\n> Right. What I'm guessing is that it sees the noreturn on die(), but then\n> doesn't actually look inside die to confirm that the noreturn is upheld.\n> You could test that with:\n>\n> #include \n> void __declspec(noreturn) die(void);\n> void die(void) { }\n> int main(void) { die(); }\n>\n> If it doesn't complain, then I am right. If it does, then it is\n> something magic with the function pointer (presumably it decides that\n> the function pointer could exit, so it stops the analysis and gives your\n> code the benefit of the doubt).\n\nIt seems that you are right. It doesn't complain here.\n\n>> The arguments against each solution I see are these:\n>> - abort() gives a run-time error instead of a compile-time warning, so\n>> breakage is trickier to detect (on GCC, which seems to be the target\n>> compiler for the vast majority of git-developers).\n>> - NORETURN_PTR might be bit big of a hammer for a small problem, as it\n>> \"pollutes\" the whole git source-tree instead of just usage.c.\n>\n> I don't know that NORETURN_PTR pollutes the whole source-tree. At least\n> no more than NORETURN does. The only functions that will need it are\n> these two function pointers.\n\nWell, it does pollute the global name space, and it's \"noise\" when\nreading the source code. But those are really very unimportant points\nIMO.\n\nAnd sure, that \"pollution/noise\" is there for the NORETURN case anyway\nfor a functional/practical reason, so you might argue that it\njustifies the presence in the NORETURN_PTR case.\n\nI'll wait a day or so and see if anyone else has any insight. If not,\nI'll go ahead and re-post a version based on the\nNORETURN_PTR-solution.\n\n-- \nErik \"kusma\" Faye-Lund\nkusmabite@gmail.com\n(+47) 986 59 656\n"},{"id":"123151","messageId":"4AAE4087.5030607@viscovery.net","threadId":"20943","inReplyTo":"40aa078e0909140532q693a7f9qc3d9b1d354cac356@mail.gmail.com","subject":"Re: [PATCH 2/2] remove NORETURN from function pointers","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-09-14T13:09:27Z","receivedAt":"2009-09-14T13:09:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Erik Faye-Lund schrieb:\n> Compiling the following code gives a warning about unreachable code,\n> so it's clear that msvc doesn't simply ignore the directive. I'm not\n> saying that anyone suggested otherwise, I just wanted to know for\n> sure.\n> \n> #include \n> #include \n> void (*exit_fun)(int) = exit;\n> void __declspec(noreturn) die(void);\n> void die(void) { exit_fun(1); }\n> int main(void) { printf(\"hello!\\n\"); die(); printf(\"world!\\n\"); }\n\nIn order to countermand any clever optimizations you should make it\n\n-void (*exit_fun)(int) = exit;\n+extern void (*exit_fun)(int);\n\n(of course, this fails to link). But if this results in only *one* warning\n(that the printf() call is unreachable), then I wouldn't bother with this\nproblem anymore, because you really should also have been warned that a\n__declspec(noreturn) function actually does return.\n\n-- Hannes\n"},{"id":"123169","messageId":"40aa078e0909140612w71ad3bdeyfa94838cb57cec19@mail.gmail.com","threadId":"20943","inReplyTo":"4AAE4087.5030607@viscovery.net","subject":"Re: [PATCH 2/2] remove NORETURN from function pointers","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-09-14T13:12:43Z","receivedAt":"2009-09-14T13:12:43Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Sep 14, 2009 at 3:09 PM, Johannes Sixt wrote:\n> Erik Faye-Lund schrieb:\n>> Compiling the following code gives a warning about unreachable code,\n>> so it's clear that msvc doesn't simply ignore the directive. I'm not\n>> saying that anyone suggested otherwise, I just wanted to know for\n>> sure.\n>>\n>> #include \n>> #include \n>> void (*exit_fun)(int) = exit;\n>> void __declspec(noreturn) die(void);\n>> void die(void) { exit_fun(1); }\n>> int main(void) { printf(\"hello!\\n\"); die(); printf(\"world!\\n\"); }\n>\n> In order to countermand any clever optimizations you should make it\n>\n> -void (*exit_fun)(int) = exit;\n> +extern void (*exit_fun)(int);\n>\n> (of course, this fails to link). But if this results in only *one* warning\n> (that the printf() call is unreachable), then I wouldn't bother with this\n> problem anymore, because you really should also have been warned that a\n> __declspec(noreturn) function actually does return.\n\nAh, good point. Still gives the warning (as well as the linker-error), though.\n\n-- \nErik \"kusma\" Faye-Lund\nkusmabite@gmail.com\n(+47) 986 59 656\n"},{"id":"123172","messageId":"4AAE42F2.30304@viscovery.net","threadId":"20943","inReplyTo":"40aa078e0909140612w71ad3bdeyfa94838cb57cec19@mail.gmail.com","subject":"Re: [PATCH 2/2] remove NORETURN from function pointers","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-09-14T13:19:46Z","receivedAt":"2009-09-14T13:19:46Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Erik Faye-Lund schrieb:\n> On Mon, Sep 14, 2009 at 3:09 PM, Johannes Sixt wrote:\n>> Erik Faye-Lund schrieb:\n>>> Compiling the following code gives a warning about unreachable code,\n>>> so it's clear that msvc doesn't simply ignore the directive. I'm not\n>>> saying that anyone suggested otherwise, I just wanted to know for\n>>> sure.\n>>>\n>>> #include \n>>> #include \n>>> void (*exit_fun)(int) = exit;\n>>> void __declspec(noreturn) die(void);\n>>> void die(void) { exit_fun(1); }\n>>> int main(void) { printf(\"hello!\\n\"); die(); printf(\"world!\\n\"); }\n>> In order to countermand any clever optimizations you should make it\n>>\n>> -void (*exit_fun)(int) = exit;\n>> +extern void (*exit_fun)(int);\n>>\n>> (of course, this fails to link). But if this results in only *one* warning\n>> (that the printf() call is unreachable), then I wouldn't bother with this\n>> problem anymore, because you really should also have been warned that a\n>> __declspec(noreturn) function actually does return.\n> \n> Ah, good point. Still gives the warning (as well as the linker-error), though.\n\n\"The\" warning? Not \"the two\" warnings? Then I suggest to stop here; MSVC\nis only half-competent with regards to noreturn.\n\n-- Hannes\n"},{"id":"123173","messageId":"40aa078e0909140626k63b9de2fu8c9411baf8200da6@mail.gmail.com","threadId":"20943","inReplyTo":"4AAE42F2.30304@viscovery.net","subject":"Re: [PATCH 2/2] remove NORETURN from function pointers","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-09-14T13:26:52Z","receivedAt":"2009-09-14T13:26:52Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, Sep 14, 2009 at 3:19 PM, Johannes Sixt wrote:\n> \"The\" warning? Not \"the two\" warnings? Then I suggest to stop here; MSVC\n> is only half-competent with regards to noreturn.\n\nThere was only one warning in this regard on MSVC - the one about\nunreachable code. And yes, MSVC is only half-competent, but it seems\nit's competence is in the half that matters in our case.\n\nDo you suggest to stop the patch-series, or to stop the testing? I'd\nprefer having NORETURN for die() etc in MSVC-builds, as it allows the\ncompiler to generate better code. I'm prefectly fine about not having\nNORETURN for the function pointers. GCC should be competent enough to\ncatch (very theoretical) errors, and we get the nice speed-improvement\non MSVC. I don't see the down-side.\n\n-- \nErik \"kusma\" Faye-Lund\nkusmabite@gmail.com\n(+47) 986 59 656\n"},{"id":"123174","messageId":"4AAE4724.1090705@viscovery.net","threadId":"20943","inReplyTo":"40aa078e0909140626k63b9de2fu8c9411baf8200da6@mail.gmail.com","subject":"Re: [PATCH 2/2] remove NORETURN from function pointers","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-09-14T13:37:40Z","receivedAt":"2009-09-14T13:37:40Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Erik Faye-Lund schrieb:\n> On Mon, Sep 14, 2009 at 3:19 PM, Johannes Sixt wrote:\n>> \"The\" warning? Not \"the two\" warnings? Then I suggest to stop here; MSVC\n>> is only half-competent with regards to noreturn.\n> \n> There was only one warning in this regard on MSVC - the one about\n> unreachable code. And yes, MSVC is only half-competent, but it seems\n> it's competence is in the half that matters in our case.\n> \n> Do you suggest to stop the patch-series, or to stop the testing?\n\nMy suggestion was about stopping the patch series.\n\nBut thinking a bit more about it, I can imagine that there are calls to\ndie() that, if it is not marked noreturn, could trigger other warnings\nwith MSVC. That would be annoying, and it's better to mark it noreturn.\n\nSo I withdraw my suggestion to stop :-)\n\n-- Hannes\n"},{"id":"123647","messageId":"7v1vlyrcef.fsf@alter.siamese.dyndns.org","threadId":"20943","inReplyTo":"4AAE4724.1090705@viscovery.net","subject":"Re: [PATCH 2/2] remove NORETURN from function pointers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-22T19:46:00Z","receivedAt":"2009-09-22T19:46:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Erik Faye-Lund schrieb:\n>> On Mon, Sep 14, 2009 at 3:19 PM, Johannes Sixt wrote:\n>>> \"The\" warning? Not \"the two\" warnings? Then I suggest to stop here; MSVC\n>>> is only half-competent with regards to noreturn.\n>> \n>> There was only one warning in this regard on MSVC - the one about\n>> unreachable code. And yes, MSVC is only half-competent, but it seems\n>> it's competence is in the half that matters in our case.\n>> \n>> Do you suggest to stop the patch-series, or to stop the testing?\n>\n> My suggestion was about stopping the patch series.\n>\n> But thinking a bit more about it, I can imagine that there are calls to\n> die() that, if it is not marked noreturn, could trigger other warnings\n> with MSVC. That would be annoying, and it's better to mark it noreturn.\n>\n> So I withdraw my suggestion to stop :-)\n\nAnything happened to this series?\n"},{"id":"123775","messageId":"40aa078e0909250656j4b791a99naf632b5b8d8e1038@mail.gmail.com","threadId":"20943","inReplyTo":"7v1vlyrcef.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] remove NORETURN from function pointers","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-09-25T13:56:27Z","receivedAt":"2009-09-25T13:56:27Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Sep 22, 2009 at 9:46 PM, Junio C Hamano wrote:\n> Johannes Sixt writes:\n>\n>> Erik Faye-Lund schrieb:\n>>> On Mon, Sep 14, 2009 at 3:19 PM, Johannes Sixt wrote:\n>>>> \"The\" warning? Not \"the two\" warnings? Then I suggest to stop here; MSVC\n>>>> is only half-competent with regards to noreturn.\n>>>\n>>> There was only one warning in this regard on MSVC - the one about\n>>> unreachable code. And yes, MSVC is only half-competent, but it seems\n>>> it's competence is in the half that matters in our case.\n>>>\n>>> Do you suggest to stop the patch-series, or to stop the testing?\n>>\n>> My suggestion was about stopping the patch series.\n>>\n>> But thinking a bit more about it, I can imagine that there are calls to\n>> die() that, if it is not marked noreturn, could trigger other warnings\n>> with MSVC. That would be annoying, and it's better to mark it noreturn.\n>>\n>> So I withdraw my suggestion to stop :-)\n>\n> Anything happened to this series?\n\nNo, I'm sorry for not updating. I'm currently on vacation, and I\ndidn't get time to fix it up before going, due to my dayjob. I'll pick\nit up again as soon as I get back home, in a bit more than a week.\n\n-- \nErik \"kusma\" Faye-Lund\nkusmabite@gmail.com\n(+47) 986 59 656\n"},{"id":"124037","messageId":"40aa078e0909301110n62ca38e2ma56d9cd38cd0b48a@mail.gmail.com","threadId":"20943","inReplyTo":"40aa078e0909250656j4b791a99naf632b5b8d8e1038@mail.gmail.com","subject":"Re: [PATCH 2/2] remove NORETURN from function pointers","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-09-30T18:10:32Z","receivedAt":"2009-09-30T18:10:32Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Sep 25, 2009 at 3:56 PM, Erik Faye-Lund\n wrote:\n>> Anything happened to this series?\n>\n> No, I'm sorry for not updating. I'm currently on vacation, and I\n> didn't get time to fix it up before going, due to my dayjob. I'll pick\n> it up again as soon as I get back home, in a bit more than a week.\n\nAlright, I fond time to fix this up a little earlier than I expected.\nA new round had been sent to the mailing list.\n\n-- \nErik \"kusma\" Faye-Lund\nkusmabite@gmail.com\n(+47) 986 59 656\n"}]}