# [PATCH] init-db: remove unused #includes 8 messages from 2015-01-14 to 2015-01-15. Participants: Alexander Kuleshov, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/38361 ## Alexander Kuleshov, 2015-01-14 17:59 Subject: [PATCH] init-db: remove unused #includes Message-ID: <1421258367-8483-1-git-send-email-kuleshovmail@gmail.com> URL: https://gitlist.dev/e/1421258367-8483-1-git-send-email-kuleshovmail%40gmail.com ``` * "cache.h" - is unnecessary because it already included at "builtin.h" * "exec_cmd.h" - was added at a47d1813 (Allow a relative builtin template directory., 15 Nov 2007). init-db used 'git_exec_path' routine from "exec_cmd.h", but later it was removed at 2de9de5e (Move code interpreting path relative to exec-dir to new function system_path()., 14 Jul 2008). So we no need in it anymore. Signed-off-by: Alexander Kuleshov --- builtin/init-db.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/builtin/init-db.c b/builtin/init-db.c index 280454a..2978b36 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -3,9 +3,7 @@ * * Copyright (C) Linus Torvalds, 2005 */ -#include "cache.h" #include "builtin.h" -#include "exec_cmd.h" #include "parse-options.h" #ifndef DEFAULT_GIT_TEMPLATE_DIR -- 2.3.0.rc0.255.g53b80d0 ``` ## Junio C Hamano, 2015-01-14 19:09 Subject: Re: [PATCH] init-db: remove unused #includes Message-ID: URL: https://gitlist.dev/e/xmqq61c99o25.fsf%40gitster.dls.corp.google.com In-Reply-To: <1421258367-8483-1-git-send-email-kuleshovmail@gmail.com> ``` Alexander Kuleshov writes: > * "cache.h" - is unnecessary because it already included at "builtin.h" > > * "exec_cmd.h" - was added at a47d1813 (Allow a relative builtin template > directory., 15 Nov 2007). init-db used 'git_exec_path' routine from > "exec_cmd.h", but later it was removed at 2de9de5e (Move code interpreting > path relative to exec-dir to new function system_path()., 14 Jul 2008). So > we no need in it anymore. > > Signed-off-by: Alexander Kuleshov > --- Makes sense. Thanks. > builtin/init-db.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/builtin/init-db.c b/builtin/init-db.c > index 280454a..2978b36 100644 > --- a/builtin/init-db.c > +++ b/builtin/init-db.c > @@ -3,9 +3,7 @@ > * > * Copyright (C) Linus Torvalds, 2005 > */ > -#include "cache.h" > #include "builtin.h" > -#include "exec_cmd.h" > #include "parse-options.h" > > #ifndef DEFAULT_GIT_TEMPLATE_DIR ``` ## Junio C Hamano, 2015-01-14 20:14 Subject: Re: [PATCH] init-db: remove unused #includes Message-ID: URL: https://gitlist.dev/e/xmqqwq4p86i6.fsf%40gitster.dls.corp.google.com In-Reply-To: ``` Junio C Hamano writes: > Alexander Kuleshov writes: > >> * "cache.h" - is unnecessary because it already included at "builtin.h" >> >> * "exec_cmd.h" - was added at a47d1813 (Allow a relative builtin template >> directory., 15 Nov 2007). init-db used 'git_exec_path' routine from >> "exec_cmd.h", but later it was removed at 2de9de5e (Move code interpreting >> path relative to exec-dir to new function system_path()., 14 Jul 2008). So >> we no need in it anymore. >> >> Signed-off-by: Alexander Kuleshov >> --- > > Makes sense. Thanks. Actually, not. 2de9de5e does make it unnecessary to refer to git_exec_path(), but in order to reference the newly introduced system_path(), which is declared in exec_cmd.h, the file still would have needed to include the header, so the justification for the latter does not make sense. And indeed, we still need exec_cmd.h in today's code. ``` ## Alexander Kuleshov, 2015-01-15 10:13 Subject: Re: [PATCH] init-db: remove unused #includes Message-ID: URL: https://gitlist.dev/e/CANCZXo5UY80327pvjzYkJXroH_WZdW36LhSANK0O7GGVQLF%3DAA%40mail.gmail.com In-Reply-To: ``` Hello Junio, yes right, missed system_path usage. But it's strange, code still compiles successfully without exec_cmd.h. ``` ## Jeff King, 2015-01-15 10:55 Subject: Re: [PATCH] init-db: remove unused #includes Message-ID: <20150115105558.GA13536@peff.net> URL: https://gitlist.dev/e/20150115105558.GA13536%40peff.net In-Reply-To: ``` On Thu, Jan 15, 2015 at 04:13:00PM +0600, Alexander Kuleshov wrote: > yes right, missed system_path usage. But it's strange, code still > compiles successfully without exec_cmd.h. Sort of. With your patch I get: builtin/init-db.c: In function ‘copy_templates’: builtin/init-db.c:127:3: warning: implicit declaration of function ‘system_path’ [-Wimplicit-function-declaration] template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR); ^ builtin/init-db.c:127:26: warning: assignment makes pointer from integer without a cast template_dir = to_free = system_path(DEFAULT_GIT_TEMPLATE_DIR); Those are just warnings, and on some platforms the resulting code will be fine, but it does violate the C standard to not have a valid declaration in this case (it defaults to an int return type, which is wrong; but simply as a matter of style we do not like implicit declarations even when they are technically correct). If you are going to be doing refactoring, I'd recommend building with "-Werror" in your CFLAGS to catch such problems. FWIW, the full set of CFLAGS I use (which I specify in config.mak) is: CFLAGS += -Wall -Werror CFLAGS += -Wno-format-zero-length CFLAGS += -Wdeclaration-after-statement CFLAGS += -Wpointer-arith CFLAGS += -Wstrict-prototypes CFLAGS += -Wold-style-declaration -Peff ``` ## Junio C Hamano, 2015-01-15 20:36 Subject: Re: [PATCH] init-db: remove unused #includes Message-ID: URL: https://gitlist.dev/e/xmqqsifb6atr.fsf%40gitster.dls.corp.google.com In-Reply-To: <20150115105558.GA13536@peff.net> ``` Jeff King writes: > FWIW, the full set of CFLAGS I use (which I specify in config.mak) is: > > CFLAGS += -Wall -Werror > CFLAGS += -Wno-format-zero-length > CFLAGS += -Wdeclaration-after-statement > CFLAGS += -Wpointer-arith > CFLAGS += -Wstrict-prototypes > CFLAGS += -Wold-style-declaration > > -Peff I think I have no-pointer-to-int-cast, old-style-definition and vla in addition to the above. ``` ## Jeff King, 2015-01-15 22:31 Subject: Re: [PATCH] init-db: remove unused #includes Message-ID: <20150115223112.GB19021@peff.net> URL: https://gitlist.dev/e/20150115223112.GB19021%40peff.net In-Reply-To: ``` On Thu, Jan 15, 2015 at 12:36:00PM -0800, Junio C Hamano wrote: > Jeff King writes: > > > FWIW, the full set of CFLAGS I use (which I specify in config.mak) is: > > > > CFLAGS += -Wall -Werror > > CFLAGS += -Wno-format-zero-length > > CFLAGS += -Wdeclaration-after-statement > > CFLAGS += -Wpointer-arith > > CFLAGS += -Wstrict-prototypes > > CFLAGS += -Wold-style-declaration > > I think I have no-pointer-to-int-cast, old-style-definition and vla > in addition to the above. Thanks, I added the latter two to my setup. But what is the purpose of turning off pointer-to-int warnings? It seems like those are a good indication of a sloppy construct (and AFAICT, we do not have any code which triggers on it). -Peff ``` ## Junio C Hamano, 2015-01-15 23:40 Subject: Re: [PATCH] init-db: remove unused #includes Message-ID: URL: https://gitlist.dev/e/xmqqlhl34nqj.fsf%40gitster.dls.corp.google.com In-Reply-To: <20150115223112.GB19021@peff.net> ``` Jeff King writes: > On Thu, Jan 15, 2015 at 12:36:00PM -0800, Junio C Hamano wrote: > >> Jeff King writes: >> >> > FWIW, the full set of CFLAGS I use (which I specify in config.mak) is: >> > >> > CFLAGS += -Wall -Werror >> > CFLAGS += -Wno-format-zero-length >> > CFLAGS += -Wdeclaration-after-statement >> > CFLAGS += -Wpointer-arith >> > CFLAGS += -Wstrict-prototypes >> > CFLAGS += -Wold-style-declaration >> >> I think I have no-pointer-to-int-cast, old-style-definition and vla >> in addition to the above. > > Thanks, I added the latter two to my setup. But what is the purpose of > turning off pointer-to-int warnings? It seems like those are a good > indication of a sloppy construct (and AFAICT, we do not have any code > which triggers on it). It probably a remnant from olden days; perhaps we used to have a code that stuffs a pointer value to an int field used as a hash key or something. As you said, there is no need for disabling that check in today's code. Thanks for catching. ```