{"thread":{"id":"3633","subject":"[PATCH] Trivial warning fix for imap-send.c","startedAt":"2006-03-11T19:29:54Z","lastAt":"2006-03-13T16:37:07Z","messageCount":20,"participants":["Art Haas","Mark Wooding","Junio C Hamano","Timo Hirvonen","Linus Torvalds","A Large Angry SCM","H. Peter Anvin","Jeff King","Olivier Galibert"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"17461","messageId":"20060311192954.GQ16135@artsapartment.org","threadId":"3633","inReplyTo":null,"subject":"[PATCH] Trivial warning fix for imap-send.c","fromName":"Art Haas","fromEmail":"ahaas@airmail.net","sentAt":"2006-03-11T19:29:54Z","receivedAt":"2006-03-11T19:29:54Z","isPatch":true,"sender":{"key":"ahaas@airmail.net","avatar":null},"body":"Hi.\n\nAfter my 'git' repo this morning and building I noticed a GCC warning\nabout a missing sentinel in this file. A scan of the libc docs says\nthat execl() needs to end with a terminating NULL, as the miniscule\nchange below does, and recompliation with GCC removed the warning.\n\nArt Haas\n\nSigned-off-by: Art Haas \n\ndiff --git a/imap-send.c b/imap-send.c\nindex fddaac0..203284d 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -945,7 +945,7 @@ imap_open_store( imap_server_conf_t *srv\n \t\t\t\t_exit( 127 );\n \t\t\tclose( a[0] );\n \t\t\tclose( a[1] );\n-\t\t\texecl( \"/bin/sh\", \"sh\", \"-c\", srvc->tunnel, 0 );\n+\t\t\texecl( \"/bin/sh\", \"sh\", \"-c\", srvc->tunnel, NULL );\n \t\t\t_exit( 127 );\n \t\t}\n \n-- \nMan once surrendering his reason, has no remaining guard against absurdities\nthe most monstrous, and like a ship without rudder, is the sport of every wind.\n\n-Thomas Jefferson to James Smith, 1822\n"},{"id":"17464","messageId":"slrne17urp.fr9.mdw@metalzone.distorted.org.uk","threadId":"3633","inReplyTo":"20060311192954.GQ16135@artsapartment.org","subject":"Re: [PATCH] Trivial warning fix for imap-send.c","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-03-12T10:44:09Z","receivedAt":"2006-03-12T10:44:09Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"\"Art Haas\" wrote:\n\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -945,7 +945,7 @@ imap_open_store( imap_server_conf_t *srv\n> \t\t\t\t_exit( 127 );\n> \t\t\tclose( a[0] );\n> \t\t\tclose( a[1] );\n> -\t\t\texecl( \"/bin/sh\", \"sh\", \"-c\", srvc->tunnel, 0 );\n> +\t\t\texecl( \"/bin/sh\", \"sh\", \"-c\", srvc->tunnel, NULL );\n> \t\t\t_exit( 127 );\n> \t\t}\n\nThis is not the right fix. NULL can be simply a #define for 0 (see\n6.3.2.3#3 and 7.17). You need to write (char *)0 or (char *)NULL. I\nprefer to avoid the macro NULL entirely, since its misleading behaviour\nis precisely what got us into this mess.\n\n-- [mdw]\n"},{"id":"17466","messageId":"7v7j6zgaxx.fsf@assigned-by-dhcp.cox.net","threadId":"3633","inReplyTo":"slrne17urp.fr9.mdw@metalzone.distorted.org.uk","subject":"Re: [PATCH] Trivial warning fix for imap-send.c","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-03-12T11:27:22Z","receivedAt":"2006-03-12T11:27:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Wooding writes:\n\n> This is not the right fix. NULL can be simply a #define for 0 (see\n> 6.3.2.3#3 and 7.17). You need to write (char *)0 or (char *)NULL. I\n> prefer to avoid the macro NULL entirely, since its misleading behaviour\n> is precisely what got us into this mess.\n\nPatches welcome. We have about 15 or so such instances.\n\n$ git grep -n -H 'execl[_a-z]*(' '*.c'\ncat-file.c:139:\t\t\treturn execl_git_cmd(\"ls-tree\", argv[2], NULL);\nconnect.c:547:\t\texeclp(git_proxy_command, git_proxy_command, host, port, NULL);\nconnect.c:646:\t\t\texeclp(ssh, ssh_basename, host, command, NULL);\nconnect.c:654:\t\t\texeclp(\"sh\", \"sh\", \"-c\", command, NULL);\ndaemon.c:263:\texecl_git_cmd(\"upload-pack\", \"--strict\", timeout_buf, \".\", NULL);\nexec_cmd.c:97:int execl_git_cmd(const char *cmd,...)\nfetch-clone.c:32:\t\texecl_git_cmd(\"index-pack\", \"-o\", idx, pack_tmp_name, NULL);\nfetch-clone.c:109:\t\texecl_git_cmd(\"unpack-objects\", quiet ? \"-q\" : NULL, NULL);\ngit.c:256:\texeclp(\"man\", \"man\", page, NULL);\nimap-send.c:948:\t\t\texecl( \"/bin/sh\", \"sh\", \"-c\", srvc->tunnel, NULL );\nmerge-index.c:18:\t\texeclp(pgm, arguments[0],\npager.c:14:\texeclp(prog, prog, NULL);\nrsh.c:106:\t\texeclp(ssh, ssh_basename, host, command, NULL);\nupload-pack.c:92:\texecl_git_cmd(\"pack-objects\", \"--stdout\", NULL);\n"},{"id":"17468","messageId":"slrne18aae.fr9.mdw@metalzone.distorted.org.uk","threadId":"3633","inReplyTo":"7v7j6zgaxx.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] Use explicit pointers for execl...() sentinels.","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-03-12T13:59:42Z","receivedAt":"2006-03-12T13:59:42Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"A terminator of `0' (or `NULL', which might well expand to `0') gets\npassed as type `int' in the absence of argument type declarations (which\nis the case for execl...(), since it uses varargs). Argument passing\nconventions may differ between `int' and `char *' if, say, `int' is 32\nbits and pointers a 64; and there's no particular guarantee that a null\npointer has all-bits-zero anyway.\n\nSigned-off-by: Mark Wooding \n\n---\n\nJunio C Hamano wrote:\n\n> Patches welcome. We have about 15 or so such instances.\n\nSo we do! ;-) \n\n---\n\n cat-file.c | 2 +-\n connect.c | 6 +++---\n daemon.c | 2 +-\n fetch-clone.c | 4 ++--\n git.c | 2 +-\n imap-send.c | 2 +-\n merge-index.c | 2 +-\n pager.c | 2 +-\n rsh.c | 2 +-\n upload-pack.c | 2 +-\n 10 files changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/cat-file.c b/cat-file.c\nindex 1a613f3..6310787 100644\n--- a/cat-file.c\n+++ b/cat-file.c\n@@ -136,7 +136,7 @@ int main(int argc, char **argv)\n \n \t\t/* custom pretty-print here */\n \t\tif (!strcmp(type, \"tree\"))\n-\t\t\treturn execl_git_cmd(\"ls-tree\", argv[2], NULL);\n+\t\t\treturn execl_git_cmd(\"ls-tree\", argv[2], (char*)0);\n \n \t\tbuf = read_sha1_file(sha1, type, &size);\n \t\tif (!buf)\ndiff --git a/connect.c b/connect.c\nindex 3f2d65c..a86a111 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -544,7 +544,7 @@ static int git_proxy_connect(int fd[2], \n \t\tclose(pipefd[0][1]);\n \t\tclose(pipefd[1][0]);\n \t\tclose(pipefd[1][1]);\n-\t\texeclp(git_proxy_command, git_proxy_command, host, port, NULL);\n+\t\texeclp(git_proxy_command, git_proxy_command, host, port, (char *)0);\n \t\tdie(\"exec failed\");\n \t}\n \tfd[0] = pipefd[0][0];\n@@ -643,7 +643,7 @@ int git_connect(int fd[2], char *url, co\n \t\t\t\tssh_basename = ssh;\n \t\t\telse\n \t\t\t\tssh_basename++;\n-\t\t\texeclp(ssh, ssh_basename, host, command, NULL);\n+\t\t\texeclp(ssh, ssh_basename, host, command, (char *)0);\n \t\t}\n \t\telse {\n \t\t\tunsetenv(ALTERNATE_DB_ENVIRONMENT);\n@@ -651,7 +651,7 @@ int git_connect(int fd[2], char *url, co\n \t\t\tunsetenv(GIT_DIR_ENVIRONMENT);\n \t\t\tunsetenv(GRAFT_ENVIRONMENT);\n \t\t\tunsetenv(INDEX_ENVIRONMENT);\n-\t\t\texeclp(\"sh\", \"sh\", \"-c\", command, NULL);\n+\t\t\texeclp(\"sh\", \"sh\", \"-c\", command, (char *)0);\n \t\t}\n \t\tdie(\"exec failed\");\n \t}\ndiff --git a/daemon.c b/daemon.c\nindex a1ccda3..13d3974 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -260,7 +260,7 @@ static int upload(char *dir)\n \tsnprintf(timeout_buf, sizeof timeout_buf, \"--timeout=%u\", timeout);\n \n \t/* git-upload-pack only ever reads stuff, so this is safe */\n-\texecl_git_cmd(\"upload-pack\", \"--strict\", timeout_buf, \".\", NULL);\n+\texecl_git_cmd(\"upload-pack\", \"--strict\", timeout_buf, \".\", (char *)0);\n \treturn -1;\n }\n \ndiff --git a/fetch-clone.c b/fetch-clone.c\nindex da1b3ff..14a1cdf 100644\n--- a/fetch-clone.c\n+++ b/fetch-clone.c\n@@ -29,7 +29,7 @@ static int finish_pack(const char *pack_\n \t\tdup2(pipe_fd[1], 1);\n \t\tclose(pipe_fd[0]);\n \t\tclose(pipe_fd[1]);\n-\t\texecl_git_cmd(\"index-pack\", \"-o\", idx, pack_tmp_name, NULL);\n+\t\texecl_git_cmd(\"index-pack\", \"-o\", idx, pack_tmp_name, (char *)0);\n \t\terror(\"cannot exec git-index-pack <%s> <%s>\",\n \t\t idx, pack_tmp_name);\n \t\texit(1);\n@@ -106,7 +106,7 @@ int receive_unpack_pack(int fd[2], const\n \t\tdup2(fd[0], 0);\n \t\tclose(fd[0]);\n \t\tclose(fd[1]);\n-\t\texecl_git_cmd(\"unpack-objects\", quiet ? \"-q\" : NULL, NULL);\n+\t\texecl_git_cmd(\"unpack-objects\", quiet ? \"-q\" : (char *)0, (char *)0);\n \t\tdie(\"git-unpack-objects exec failed\");\n \t}\n \tclose(fd[0]);\ndiff --git a/git.c b/git.c\nindex 0b40e30..8dd7933 100644\n--- a/git.c\n+++ b/git.c\n@@ -253,7 +253,7 @@ static void show_man_page(const char *gi\n \t\tpage = p;\n \t}\n \n-\texeclp(\"man\", \"man\", page, NULL);\n+\texeclp(\"man\", \"man\", page, (char *)0);\n }\n \n static int cmd_version(int argc, const char **argv, char **envp)\ndiff --git a/imap-send.c b/imap-send.c\nindex 1b38b3a..825a5cf 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -946,7 +946,7 @@ imap_open_store( imap_server_conf_t *srv\n \t\t\t\t_exit( 127 );\n \t\t\tclose( a[0] );\n \t\t\tclose( a[1] );\n-\t\t\texecl( \"/bin/sh\", \"sh\", \"-c\", srvc->tunnel, NULL );\n+\t\t\texecl( \"/bin/sh\", \"sh\", \"-c\", srvc->tunnel, (char *)0);\n \t\t\t_exit( 127 );\n \t\t}\n \ndiff --git a/merge-index.c b/merge-index.c\nindex 024196e..24d4b62 100644\n--- a/merge-index.c\n+++ b/merge-index.c\n@@ -23,7 +23,7 @@ static void run_program(void)\n \t\t\t arguments[5],\n \t\t\t arguments[6],\n \t\t\t arguments[7],\n-\t\t\t NULL);\n+\t\t\t (char *)0);\n \t\tdie(\"unable to execute '%s'\", pgm);\n \t}\n \tif (waitpid(pid, &status, 0) < 0 || !WIFEXITED(status) || WEXITSTATUS(status)) {\ndiff --git a/pager.c b/pager.c\nindex 1364e15..9da76ac 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -11,7 +11,7 @@ static void run_pager(void)\n \tif (!prog)\n \t\tprog = \"less\";\n \tsetenv(\"LESS\", \"-S\", 0);\n-\texeclp(prog, prog, NULL);\n+\texeclp(prog, prog, (char *)0);\n }\n \n void setup_pager(void)\ndiff --git a/rsh.c b/rsh.c\nindex d665269..92ace2f 100644\n--- a/rsh.c\n+++ b/rsh.c\n@@ -103,7 +103,7 @@ int setup_connection(int *fd_in, int *fd\n \t\tclose(sv[1]);\n \t\tdup2(sv[0], 0);\n \t\tdup2(sv[0], 1);\n-\t\texeclp(ssh, ssh_basename, host, command, NULL);\n+\t\texeclp(ssh, ssh_basename, host, command, (char *)0);\n \t}\n \tclose(sv[0]);\n \t*fd_in = sv[1];\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 47560c9..fbfdbcd 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -89,7 +89,7 @@ static void create_pack_file(void)\n \tdup2(fd[0], 0);\n \tclose(fd[0]);\n \tclose(fd[1]);\n-\texecl_git_cmd(\"pack-objects\", \"--stdout\", NULL);\n+\texecl_git_cmd(\"pack-objects\", \"--stdout\", (char *)0);\n \tdie(\"git-upload-pack: unable to exec git-pack-objects\");\n }\n \n\n-- [mdw]\n"},{"id":"17472","messageId":"20060312171316.39d138f8.tihirvon@gmail.com","threadId":"3633","inReplyTo":"slrne18aae.fr9.mdw@metalzone.distorted.org.uk","subject":"Re: [PATCH] Use explicit pointers for execl...() sentinels.","fromName":"Timo Hirvonen","fromEmail":"tihirvon@gmail.com","sentAt":"2006-03-12T15:13:16Z","receivedAt":"2006-03-12T15:13:16Z","isPatch":true,"sender":{"key":"tihirvon@gmail.com","avatar":null},"body":"On Sun, 12 Mar 2006 13:59:42 +0000 (UTC)\nMark Wooding wrote:\n\n> A terminator of `0' (or `NULL', which might well expand to `0') gets\n> passed as type `int' in the absence of argument type declarations (which\n> is the case for execl...(), since it uses varargs). Argument passing\n> conventions may differ between `int' and `char *' if, say, `int' is 32\n> bits and pointers a 64; and there's no particular guarantee that a null\n> pointer has all-bits-zero anyway.\n\nNULL should always be ((void *)0). What 64-bit systems declare NULL as\nplain 0 (not 0L)? How about fixing those systems instead of making the\ngit source code unreadable.\n\n-- \nhttp://onion.dynserv.net/~timo/\n"},{"id":"17473","messageId":"Pine.LNX.4.64.0603120847500.3618@g5.osdl.org","threadId":"3633","inReplyTo":"slrne17urp.fr9.mdw@metalzone.distorted.org.uk","subject":"Re: [PATCH] Trivial warning fix for imap-send.c","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-03-12T16:57:02Z","receivedAt":"2006-03-12T16:57:02Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 12 Mar 2006, Mark Wooding wrote:\n> \"Art Haas\" wrote:\n> \n> > -\t\t\texecl( \"/bin/sh\", \"sh\", \"-c\", srvc->tunnel, 0 );\n> > +\t\t\texecl( \"/bin/sh\", \"sh\", \"-c\", srvc->tunnel, NULL );\n> \n> This is not the right fix. NULL can be simply a #define for 0 (see\n> 6.3.2.3#3 and 7.17). You need to write (char *)0 or (char *)NULL. I\n> prefer to avoid the macro NULL entirely, since its misleading behaviour\n> is precisely what got us into this mess.\n\nIt's perfectly fine.\n\nQuite frankly, if you have a 64-bit C compiler that doesn't make \"NULL\" be \n\"((void *)0)\" (or equivalent - some compilers will actually have NULL as \nan intrisic, because especially if they also support C++, NULL has some \nreally magical properties there), you should switch vendors as quickly as \nhumanly possible.\n\nThe \"#define NULL 0\" practice is still _legal_ C, but that doesn't make it \nany less broken. It's K&R traditional, but git requires ANSI prototypes \nand some fancy features from modern compilers, so K&R compilers aren't \nwelcome anyway, and if their headers don't define NULL as a void pointer, \ntheir headers are simply _broken_.\n\nSo in modern C, using NULL at the end of a varargs array as a pointer is \nperfectly sane, and the extra cast is just ugly and bowing to bad \nprogramming practices and makes no sense to anybody who never saw the \nhorror that is K&R.\n\nIt's akin to trying to not using prototypes, or to trying to limit your \nexternally visible names to 7 characters. It was \"appropriate\" about two \ndecades ago, these days it's just cuddling broken setups that have been \nbroken for a long long time.\n\nBtw, the reason NULL _has_ to be a pointer (\"((void *)0)\" or otherwise) is \nsimply that if it isn't, you not only won't get reasonable varargs \nbehaviour, you'll also miss real warnings. I've seen broken code like\n\n\tint i = NULL;\n\nin my life, and if the compiler doesn't warn about that, then the compiler \nis BROKEN, and not worth supporting as a programmer.\n\n\t\t\tLinus\n"},{"id":"17475","messageId":"slrne18mq3.fr9.mdw@metalzone.distorted.org.uk","threadId":"3633","inReplyTo":"20060312171316.39d138f8.tihirvon@gmail.com","subject":"Re: [PATCH] Use explicit pointers for execl...() sentinels.","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-03-12T17:32:51Z","receivedAt":"2006-03-12T17:32:51Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"Timo Hirvonen wrote:\n\n> NULL should always be ((void *)0).\n\nSays who? I already gave chapter and verse for what NULL is required to\ndo.\n\nBesides, (void *)0 fixes /this particular/ problem, because `void *' and\n`char *' have the same representation (6.2.5#27). This wouldn't help us\nwith a putative function which takes an arbitrary number of `foo *'\npointers, since nothing guarantees that `void *' and `foo *' have\nsimilar representations. You'd have to say `(foo *)0' or `(foo *)NULL'.\n\n> What 64-bit systems declare NULL as plain 0 (not 0L)?\n\nDon't know: didn't look. 0L won't do the right thing with IL32LLP64, if\nanyone was actually crazy enough to specify such an ABI. The point is,\nthere's not much \n\n> How about fixing those systems instead of making the git source code\n> unreadable.\n\nBecause, according to the C and POSIX specs, they're not wrong.\n\nThe right fix from the point of view of a C implementation would be to\ndefine NULL to be some weird __null_pointer token which the compiler\ncould warn about whenever it was used in an untyped argument context.\n\n(Besides, I don't find bare or casted `0' unreadable. Maybe I'm just\nstrange.)\n\n-- [mdw]\n"},{"id":"17476","messageId":"slrne18of5.fr9.mdw@metalzone.distorted.org.uk","threadId":"3633","inReplyTo":"Pine.LNX.4.64.0603120847500.3618@g5.osdl.org","subject":"Re: [PATCH] Trivial warning fix for imap-send.c","fromName":"Mark Wooding","fromEmail":"mdw@distorted.org.uk","sentAt":"2006-03-12T18:01:09Z","receivedAt":"2006-03-12T18:01:09Z","isPatch":true,"sender":{"key":"mdw@distorted.org.uk","avatar":null},"body":"Linus Torvalds wrote:\n\n> So in modern C, using NULL at the end of a varargs array as a pointer is \n> perfectly sane, and the extra cast is just ugly and bowing to bad \n> programming practices and makes no sense to anybody who never saw the \n> horror that is K&R.\n\nNo! You can still get bitten. You're lucky that on common platforms\nall pointers look the same, but if you find one where `char *' (and\nhence `void *') isn't the same as `struct foo *' then, under appropriate\ncircumstances you /will/ unless you put the casts in.\n\nNow, maybe we don't care for GIT. That's your (and Junio's) call. My\nnatural approach is to work as closely as I can to the specs (and then\nthrow in hacks for platforms which /still/ don't work), though, which is\nwhy I brought the subject up.\n\n-- [mdw]\n"},{"id":"17477","messageId":"20060312200812.3fb04638.tihirvon@gmail.com","threadId":"3633","inReplyTo":"slrne18mq3.fr9.mdw@metalzone.distorted.org.uk","subject":"Re: [PATCH] Use explicit pointers for execl...() sentinels.","fromName":"Timo Hirvonen","fromEmail":"tihirvon@gmail.com","sentAt":"2006-03-12T18:08:12Z","receivedAt":"2006-03-12T18:08:12Z","isPatch":true,"sender":{"key":"tihirvon@gmail.com","avatar":null},"body":"On Sun, 12 Mar 2006 17:32:51 +0000 (UTC)\nMark Wooding wrote:\n\n> Besides, (void *)0 fixes /this particular/ problem, because `void *' and\n> `char *' have the same representation (6.2.5#27). This wouldn't help us\n> with a putative function which takes an arbitrary number of `foo *'\n> pointers, since nothing guarantees that `void *' and `foo *' have\n> similar representations. You'd have to say `(foo *)0' or `(foo *)NULL'.\n\nNULL pointer does not point to any data, it just says it's 'empty'. So\nit doesn't need to be same type pointer as specified in the function\nprototype. Pointers are just addresses, it doesn't matter from to code\ngeneration point of view whether it is (char *)0 or (void *)0.\n\n> Don't know: didn't look. 0L won't do the right thing with IL32LLP64, if\n> anyone was actually crazy enough to specify such an ABI. The point is,\n> there's not much \n\nsizeof(unsigned long) is sizeof(void *) in real world.\n\n> > How about fixing those systems instead of making the git source code\n> > unreadable.\n> \n> Because, according to the C and POSIX specs, they're not wrong.\n\nThey didn't think of 64-bit architectures back then, I suppose.\n\n> The right fix from the point of view of a C implementation would be to\n> define NULL to be some weird __null_pointer token which the compiler\n> could warn about whenever it was used in an untyped argument context.\n\nIn practice (void *)0 is good enough.\n\n> (Besides, I don't find bare or casted `0' unreadable. Maybe I'm just\n> strange.)\n\n'ugly' would have been better word than 'unreadable'.\n\n-- \nhttp://onion.dynserv.net/~timo/\n"},{"id":"17479","messageId":"4414747B.7040700@gmail.com","threadId":"3633","inReplyTo":"slrne18of5.fr9.mdw@metalzone.distorted.org.uk","subject":"Re: [PATCH] Trivial warning fix for imap-send.c","fromName":"A Large Angry SCM","fromEmail":"gitzilla@gmail.com","sentAt":"2006-03-12T19:20:27Z","receivedAt":"2006-03-12T19:20:27Z","isPatch":true,"sender":{"key":"gitzilla@gmail.com","avatar":"https://gravatar.com/avatar/354625c442439908ff3dd99757dee330e29e9df7847472384faf7a00add247fb?d=mp&s=160"},"body":"Mark Wooding wrote:\n> Linus Torvalds wrote:\n> \n>>So in modern C, using NULL at the end of a varargs array as a pointer is \n>>perfectly sane, and the extra cast is just ugly and bowing to bad \n>>programming practices and makes no sense to anybody who never saw the \n>>horror that is K&R.\n> \n> No! You can still get bitten. You're lucky that on common platforms\n> all pointers look the same, but if you find one where `char *' (and\n> hence `void *') isn't the same as `struct foo *' then, under appropriate\n> circumstances you /will/ unless you put the casts in.\n\nPlease explain how malloc() can work on such a platform. My reading of \nthe '89 ANSI C spec. finds that _ALL_ (non function) pointers _are_ \ncast-able to/from a void * and that NULL should be #defined as (void *). \nSee 3.2.2.3 and 4.1.5 if interested.\n"},{"id":"17484","messageId":"Pine.LNX.4.64.0603121457520.3618@g5.osdl.org","threadId":"3633","inReplyTo":"slrne18of5.fr9.mdw@metalzone.distorted.org.uk","subject":"Re: [PATCH] Trivial warning fix for imap-send.c","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-03-12T23:02:40Z","receivedAt":"2006-03-12T23:02:40Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 12 Mar 2006, Mark Wooding wrote:\n\n> Linus Torvalds wrote:\n> \n> > So in modern C, using NULL at the end of a varargs array as a pointer is \n> > perfectly sane, and the extra cast is just ugly and bowing to bad \n> > programming practices and makes no sense to anybody who never saw the \n> > horror that is K&R.\n> \n> No! You can still get bitten. You're lucky that on common platforms\n> all pointers look the same, but if you find one where `char *' (and\n> hence `void *') isn't the same as `struct foo *' then, under appropriate\n> circumstances you /will/ unless you put the casts in.\n\nNot relevant. Show me any system that matters.\n\nThe fact is, compilers should conform to programmers, not the other way \naround. Bending over backwards for broken systems is _wrong_. The fact \nthat there are insane build environments doesn't excuse bad manners, and \nexplicit casts that aren't needed are HORRIBLE manners.\n\nThere is no valid reason to _ever_ cast NULL pointers. \n\nBtw, the same goes for casting the result from malloc etc, which some \npeople also do. \n\nPut another way: you should not encourage insane systems, and you should \ndefinitely NOT encourage nit-picking people who read the standards in \ninsane ways and say that the standards _allow_ badly behaved build \nenvironments.\n\nIt's true that the standards _allow_ crazy build environments. Who the \nf*ck cares? Crazy and bad build environments aren't any better for being \nallowed by the standard. Screw them. Call them names. And refuse to work \nwith them.\n\n\t\tLinus\n"},{"id":"17494","messageId":"4414E000.9030902@zytor.com","threadId":"3633","inReplyTo":"4414747B.7040700@gmail.com","subject":"Re: [PATCH] Trivial warning fix for imap-send.c","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2006-03-13T02:59:12Z","receivedAt":"2006-03-13T02:59:12Z","isPatch":true,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"A Large Angry SCM wrote:\n> Mark Wooding wrote:\n> \n>> Linus Torvalds wrote:\n>>\n>>> So in modern C, using NULL at the end of a varargs array as a pointer \n>>> is perfectly sane, and the extra cast is just ugly and bowing to bad \n>>> programming practices and makes no sense to anybody who never saw the \n>>> horror that is K&R.\n>>\n>> No! You can still get bitten. You're lucky that on common platforms\n>> all pointers look the same, but if you find one where `char *' (and\n>> hence `void *') isn't the same as `struct foo *' then, under appropriate\n>> circumstances you /will/ unless you put the casts in.\n> \n> Please explain how malloc() can work on such a platform. My reading of \n> the '89 ANSI C spec. finds that _ALL_ (non function) pointers _are_ \n> cast-able to/from a void * and that NULL should be #defined as (void *). \n> See 3.2.2.3 and 4.1.5 if interested.\n\nConsider the non-hypothetical example of a word-addressed machine, which \nhas to have extra bits in a subword pointer like char *. The C standard \nrequires that void * has those bits as well, but it doesn't means that \nany void * can be cast to any arbitrary pointer -- the opposite, \nhowever, is required.\n\n\t-hpa\n"},{"id":"17495","messageId":"20060313033121.GA14601@coredump.intra.peff.net","threadId":"3633","inReplyTo":"20060312200812.3fb04638.tihirvon@gmail.com","subject":"Re: [PATCH] Use explicit pointers for execl...() sentinels.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2006-03-13T03:31:21Z","receivedAt":"2006-03-13T03:31:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 12, 2006 at 08:08:12PM +0200, Timo Hirvonen wrote:\n\n> NULL pointer does not point to any data, it just says it's 'empty'. So\n> it doesn't need to be same type pointer as specified in the function\n> prototype. Pointers are just addresses, it doesn't matter from to code\n> generation point of view whether it is (char *)0 or (void *)0.\n\nSorry, but I think you're wrong according to the C standard. Pointers of \ndifferent types do NOT have to share the same representation (e.g.,\nthere have been some platforms where char* and int* were different\nsizes). A void pointer must be capable of representing any type of\npointer (for example, holding the largest possible type). However, if\nsizeof(void *) == 8 and sizeof(char *) == 4, you have a problem with\nvariadic functions which are expecting to pull 4 byte off the stack. \n\nIn a non-variadic function, the compiler would do the right implicit\ncasting. In a variadic function, it can't. \n\nThe real question is, does git want to care about portability to such\nplatforms.\n\nIf you remain unconvinced, I can try to find chapter and verse of the\nstandard.\n\n> sizeof(unsigned long) is sizeof(void *) in real world.\n\nAre you saying that because it encompasses all of the platforms you've\nworked on, or do you have some evidence that it is largely the case? It\ncertainly isn't guaranteed by the C standard.\n\n> > Because, according to the C and POSIX specs, they're not wrong.\n> They didn't think of 64-bit architectures back then, I suppose.\n\nNo, they did think of those issues; they intentionally left such sizing\nup to the implementation to allow C to grow with the hardware. Mostly\nyou don't have to care, but as I said, typing with variadic functions is\na pain.\n\n-Peff\n"},{"id":"17496","messageId":"20060313033805.GB14601@coredump.intra.peff.net","threadId":"3633","inReplyTo":"4414747B.7040700@gmail.com","subject":"Re: [PATCH] Trivial warning fix for imap-send.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2006-03-13T03:38:05Z","receivedAt":"2006-03-13T03:38:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 12, 2006 at 11:20:27AM -0800, A Large Angry SCM wrote:\n\n> >No! You can still get bitten. You're lucky that on common platforms\n> >all pointers look the same, but if you find one where `char *' (and\n> >hence `void *') isn't the same as `struct foo *' then, under appropriate\n> >circumstances you /will/ unless you put the casts in.\n> \n> Please explain how malloc() can work on such a platform. My reading of \n> the '89 ANSI C spec. finds that _ALL_ (non function) pointers _are_ \n> cast-able to/from a void * and that NULL should be #defined as (void *). \n> See 3.2.2.3 and 4.1.5 if interested.\n\nI think Linus has cut to the heart of the discussion (that it's worth\ngit maintainers' sanity not to worry about such problems). However, for\npedantry's sake, this is how malloc works:\n\nA void pointer is guaranteed to be able to hold any type of pointer\n(either char * or struct foo * or whatever). The declaration of malloc\nindicates a return of void *. On a platform where it matters, the\ncompiler generates code so that \n struct foo *bar = malloc(100);\nconverts the void * pointer into the correct size (in the same way that\nassigning between differently sized integers works).\n\nThis breaks down with variadic functions, which have no typing\ninformation. So doing this:\n execl(\"foo\", \"bar\", my_struct_foo);\ndoesn't give the compiler a chance to do the implicit cast and you get\nsubtle breakage (in the same way that you would if you passed a long to\na variadic function expecting a short).\n\n-Peff\n"},{"id":"17501","messageId":"4414F6B1.9080107@gmail.com","threadId":"3633","inReplyTo":"4414E000.9030902@zytor.com","subject":"Re: [PATCH] Trivial warning fix for imap-send.c","fromName":"A Large Angry SCM","fromEmail":"gitzilla@gmail.com","sentAt":"2006-03-13T04:36:01Z","receivedAt":"2006-03-13T04:36:01Z","isPatch":true,"sender":{"key":"gitzilla@gmail.com","avatar":"https://gravatar.com/avatar/354625c442439908ff3dd99757dee330e29e9df7847472384faf7a00add247fb?d=mp&s=160"},"body":"H. Peter Anvin wrote:\n> A Large Angry SCM wrote:\n>> Mark Wooding wrote:\n>>\n>>> Linus Torvalds wrote:\n>>>\n>>>> So in modern C, using NULL at the end of a varargs array as a \n>>>> pointer is perfectly sane, and the extra cast is just ugly and \n>>>> bowing to bad programming practices and makes no sense to anybody \n>>>> who never saw the horror that is K&R.\n>>>\n>>> No! You can still get bitten. You're lucky that on common platforms\n>>> all pointers look the same, but if you find one where `char *' (and\n>>> hence `void *') isn't the same as `struct foo *' then, under appropriate\n>>> circumstances you /will/ unless you put the casts in.\n>>\n>> Please explain how malloc() can work on such a platform. My reading of \n>> the '89 ANSI C spec. finds that _ALL_ (non function) pointers _are_ \n>> cast-able to/from a void * and that NULL should be #defined as (void \n>> *). See 3.2.2.3 and 4.1.5 if interested.\n> \n> Consider the non-hypothetical example of a word-addressed machine, which \n> has to have extra bits in a subword pointer like char *. The C standard \n> requires that void * has those bits as well, but it doesn't means that \n> any void * can be cast to any arbitrary pointer -- the opposite, \n> however, is required.\n\nANSI X3.159-1989\n\n3.2.2.3 Pointers\n\tA pointer to *void* may be converted to or from a pointer to any \nincomplete or object type. A pointer to any incomplete or object type \nmay be converted to a pointer to *void* and back again; the result shall \ncompare equal to the original pointer.\n\nFor any qualifier /q/, a pointer to a non-/q/-qualified type may be \nconverted to a pointer to the /q/-qualified version of the type; the \nvalues stored in the original and converted pointers shall compare equal.\n\nIn integral constant expression with value 0, or such an expression cast \nto type void *, is called a /null pointer constant.[*33*] \nIf a null pointer constant is assigned to or compared for equality to a \npointer, the constant is converted to a pointer of that type. Such a \npointer, called a /null pointer/, is guaranteed to compare unequal to a \npointer to any object or function.\n\nTwo null pointers, converted through possibly different sequences of \ncasts to pointer types, shall compare equal.\n\n[*33*] the macro *NULL* is defined in as a null pointer \nconstant; see 4.1.5.\n"},{"id":"17505","messageId":"Pine.LNX.4.64.0603122103440.3618@g5.osdl.org","threadId":"3633","inReplyTo":"4414F6B1.9080107@gmail.com","subject":"Re: [PATCH] Trivial warning fix for imap-send.c","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-03-13T05:22:16Z","receivedAt":"2006-03-13T05:22:16Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 12 Mar 2006, A Large Angry SCM wrote:\n> \n> 3.2.2.3 Pointers\n> \tA pointer to *void* may be converted to or from a pointer to any\n> incomplete or object type. A pointer to any incomplete or object type may be\n> converted to a pointer to *void* and back again; the result shall compare\n> equal to the original pointer.\n\nLarge, you're missing the point.\n\n\"void *\" is guaranteed to be a _superset_ of all pointers.\n\nBut that dos not mean that any \"void *\" pointer can be cast to any other \npointer. BUT IF IT STARTED OUT AS A POINTER OF THAT TYPE, IT'S GUARANTEED \nTHAT IT CAN BE CAST _BACK_ TO THAT TYPE.\n\n(And furthermore, NULL is special in that it will always compare equal \nregardless of how it has ever been cast).\n\nThis means, for example, that it's perfectly legal for a C implementation \nto have a 128-bit \"void *\", where the low bits are the \"real pointer\" and \nthe high 64 bits are the \"type descriptor\". You could only cast such a \npointer to that proper type, but you could not cast it to any other type \n(except for the special case of NULL).\n\nMy argument boils down to the fact that we don't care one whit about those \ntheoretical architectures. It so happens that ia64 function pointers are \nsometimes described this way (due to totally broken reasons - don't ask), \nand that function pointers could indeed be seen as 128-bit quantities. But \nthat is such a horribly broken thing, that what compilers on ia64 actually \ndo is to instead of having a 128-bit \"void *\" (which would be legal per \nthe standard), they make function pointers actually point to the function \ndescription (128-bit datum) rather than the actual start of the function.\n\n(I think. I forget the exact details. I think the whole architecture is a \ntotal mess, and should never have been done in the firstplace).\n\nSimilarly, there are certain tagged architectures where the pointer \nactually contains the type it points to, and again, C _allows_ that, and \nif you want to be strictly conforming, you can't do certain things that \nseem obviously correct.\n\nHOWEVER. The undeniable fact is that no sane architecture that anybody \ncares about today (and that, in turn, implies that nobody will care about \nit in the next quarter century - these things have a tendency to \nre-inforce themselves) actually does that. \n\nAnother example is two's complement. C as a language actually allows other \ntype representations than two's complement for integers, and there's lots \nof verbiage in the standard about how overflow is undefined etc. Then they \ngo to pains to explain how \"unsigned\" integers are guaranteed to behave as \nif the machine was a regular binary machine, even though the language \nlawyers in general went to great pain to make it clear that if the integer \nrepresentation is binary-packed-decimal, it's still legal from a C \nstanpoint.\n\nBut again, nobody sane would ever care. The likelihood that we'll see a \nternary machine in the next few decades is pretty damn small, because \nwhile the C standard allows for something else, it would be painful in the \nextreme for anybody to actually convert all the programs that effectively \ndepend on 8-bit bytes etc.\n\nSo again, in _theory_ the C standard works for some really odd crap out \nthere. In practice, there are only certain pretty standard setups (ILP32, \nI32LP64, IL32P64), and some old ones (I16LP32) that nobody cares about, \nand then the really odd ones (36-bit word-addressable monsters where char, \nshort, int, long and pointer are all the same size) that have a C \ncompiler, but that you will never be able to port _any_ normal program \nto..\n\nIn other words, the C standard allows some really strange stuff. Trying to \neven worry about it is just not worth it. It often makes the code just \nmuch harder to read for absolutely zero gain.\n\nSo in practice, the strangest setup you'll ever really care about is \nactually Windows. And it's strange because it can have a totally broken \nsize model (IL32LLP64 - although I think that's usually just a compiler \nswitch), and because it has such strange system libraries and filesystem \nbehaviour (which is sadly more than just a compiler switch).\n\nEven windows (or, perhaps, Windows _in_particular_) will never have things \nlike a \"char\" that isn't 8 bits, etc that could be possible in theory if \nyou were to just read the C standard.\n\n\t\tLinus\n"},{"id":"17508","messageId":"44151330.7020905@zytor.com","threadId":"3633","inReplyTo":"Pine.LNX.4.64.0603122103440.3618@g5.osdl.org","subject":"Re: [PATCH] Trivial warning fix for imap-send.c","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2006-03-13T06:37:36Z","receivedAt":"2006-03-13T06:37:36Z","isPatch":true,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"On \"real\" machines, the biggest reason you'd care is that a lot of \ncompilers, *especially* in C++ mode, really still define NULL as \"0\"; \nostensibly because defining it as \"((void *)0)\" breaks some obscure C++ \ncasting rule.\n\n\t-hpa\n"},{"id":"17509","messageId":"4415141F.2080604@zytor.com","threadId":"3633","inReplyTo":"20060313033805.GB14601@coredump.intra.peff.net","subject":"Re: [PATCH] Trivial warning fix for imap-send.c","fromName":"H. Peter Anvin","fromEmail":"hpa@zytor.com","sentAt":"2006-03-13T06:41:35Z","receivedAt":"2006-03-13T06:41:35Z","isPatch":true,"sender":{"key":"hpa@zytor.com","avatar":null},"body":"Jeff King wrote:\n> \n> I think Linus has cut to the heart of the discussion (that it's worth\n> git maintainers' sanity not to worry about such problems). However, for\n> pedantry's sake, this is how malloc works:\n> \n> A void pointer is guaranteed to be able to hold any type of pointer\n> (either char * or struct foo * or whatever). The declaration of malloc\n> indicates a return of void *. On a platform where it matters, the\n> compiler generates code so that \n> struct foo *bar = malloc(100);\n> converts the void * pointer into the correct size (in the same way that\n> assigning between differently sized integers works).\n> \n\nFurthermore, the return value of malloc() (calloc, realloc, ...) is \nguaranteed to be a value suitable for casting to any pointer type for \nwhich at least one object can fit in the allocated memory block.\n\n\t-hpa\n"},{"id":"17511","messageId":"Pine.LNX.4.64.0603122245360.3618@g5.osdl.org","threadId":"3633","inReplyTo":"44151330.7020905@zytor.com","subject":"Re: [PATCH] Trivial warning fix for imap-send.c","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-03-13T06:46:05Z","receivedAt":"2006-03-13T06:46:05Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sun, 12 Mar 2006, H. Peter Anvin wrote:\n>\n> On \"real\" machines, the biggest reason you'd care is that a lot of compilers,\n> *especially* in C++ mode, really still define NULL as \"0\"; ostensibly because\n> defining it as \"((void *)0)\" breaks some obscure C++ casting rule.\n\nAgreed. gcc has fixed that rule, but others have not. Don't compile git \nas C++.\n\n\t\tLinus\n"},{"id":"17527","messageId":"20060313163707.GB87487@dspnet.fr.eu.org","threadId":"3633","inReplyTo":"44151330.7020905@zytor.com","subject":"Re: [PATCH] Trivial warning fix for imap-send.c","fromName":"Olivier Galibert","fromEmail":"galibert@pobox.com","sentAt":"2006-03-13T16:37:07Z","receivedAt":"2006-03-13T16:37:07Z","isPatch":true,"sender":{"key":"galibert@pobox.com","avatar":null},"body":"On Sun, Mar 12, 2006 at 10:37:36PM -0800, H. Peter Anvin wrote:\n> On \"real\" machines, the biggest reason you'd care is that a lot of \n> compilers, *especially* in C++ mode, really still define NULL as \"0\"; \n> ostensibly because defining it as \"((void *)0)\" breaks some obscure C++ \n> casting rule.\n\nNot obscure, just a religious issue. Somehow in the creation of the\nC++ standard the definition of void * got changed from \"generic\npointer\" to something else I've been unable to fathom. That\ndefinition, whatever it is, justifies forbidding implicit casts from\nvoid * to anything else. Some of the priests of the new definition\nconsider the existence in C of a usable generic pointer type to be a\nfailing of the language too.\n\n OG.\n"}]}