Re: [PATCH] run-command: introduce CHILD_PROCESS_INIT
- From
Johannes Sixt <j6t@kdbg.org>
- Date
- Aug 17, 2014, 07:46 UTC
- Message-ID
- <53F05DE2.5080806@kdbg.org>
- In-Reply-To
- <53EFE15B.7030805@web.de>
Am 17.08.2014 00:55, schrieb René Scharfe:
Show 5 quoted lines
> Most struct child_process variables are cleared using memset right after > declaration. Provide a macro, CHILD_PROCESS_INIT, that can be used to > initialize them statically instead. That's shorter, doesn't require a > function call and is slightly more readable (especially given that we > already have similar macros like STRBUF_INIT, ARGV_ARRAY_INIT etc.).
This is a step in the right direction, IMO. This way to initialize the struct feels mucth better because it does not depend on that the bit pattern of the NULL pointer is all zeros.
Show 38 quoted lines
> Signed-off-by: Rene Scharfe <l.s.r@web.de> > --- > Documentation/technical/api-run-command.txt | 4 ++-- > archive-tar.c | 3 +-- > builtin/add.c | 3 +-- > builtin/commit.c | 3 +-- > builtin/help.c | 3 +-- > builtin/merge.c | 3 +-- > builtin/notes.c | 3 +-- > builtin/remote-ext.c | 3 +-- > builtin/repack.c | 3 +-- > builtin/replace.c | 4 ++-- > builtin/verify-pack.c | 3 +-- > bundle.c | 6 ++---- > connected.c | 3 +-- > convert.c | 3 +-- > credential-cache.c | 3 +-- > credential.c | 3 +-- > daemon.c | 8 +++----- > diff.c | 3 +-- > editor.c | 3 +-- > fetch-pack.c | 3 +-- > gpg-interface.c | 6 ++---- > http-backend.c | 3 +-- > http.c | 3 +-- > imap-send.c | 2 +- > prompt.c | 3 +-- > remote-curl.c | 3 +-- > remote-testsvn.c | 3 +-- > run-command.h | 2 ++ > send-pack.c | 3 +-- > submodule.c | 21 +++++++-------------- > test-run-command.c | 4 +--- > test-subprocess.c | 3 +-- > transport.c | 12 ++++-------- > upload-pack.c | 3 +-- > wt-status.c | 3 +-- > 35 files changed, 51 insertions(+), 93 deletions(-)
You missed a few instances in builtin/receive-pack.c. Is this deliberate?
Another one is in transport-helper.c::fetch_with_import() that is currently initialized in get_importer(), and another one in push_refs_with_export() that is initialized in get_exporter().
Should the static struct child_process variables in pager.c and connect.c use the macro? It would just be for completeness, but does not change the correctness. I dunno.
Show 9 quoted lines
> diff --git a/run-command.h b/run-command.h
> index ea73de3..95a8fb8 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -44,6 +44,8 @@ struct child_process {
> unsigned clean_on_exit:1;
> };
>
> +#define CHILD_PROCESS_INIT { NULL }I would have expected this to read
#define CHILD_PROCESS_INIT { NULL, ARGV_ARRAY_INIT }It does change the bit pattern of the initialized struct child_process because ARGV_ARRAY_INIT uses a non-NULL address. But IMHO ARGV_ARRAY_INIT should be used here as a defensive measure.
-- Hannes