Re: [RFC/PATCH 01/48] builtin/apply: avoid parameter shadowing 'p_value' global

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 9, 2016, 23:27 UTC
Message-ID: <xmqqlh5rfd0t.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1457545756-20616-2-git-send-email-chriscool@tuxfamily.org>

Christian Couder <christian.couder@gmail.com> writes:

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  builtin/apply.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)

To be honest, I have to say that I do not like this change from readability's point of view. Once the global p_value becomes a field in a "apply state" structure, functions will get a pointer to the struct as a parameter, reference to it would be spelled as "astate->p_value", while a local variable or a parameter would be "p_value" and there is no shadowing issue.

Also, you wouldn't be able to catch a misconversion in this patch only from the patch text, if the conversion is done this way, would you? The patch may change the parameter p_value to p_v, and change two references to p_value also to p_v, but it may leave another reference to p_value that originally referred to the parameter as-is, making it refer to the global, which would be a new bug introduced by this conversion patch, but that remaining reference would not show up in the patch.