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

From: Duy Nguyen <pclouds@gmail.com>
Date: Mar 10, 2016, 00:54 UTC
Message-ID: <CACsJy8Boz1XLv7yh5GsOQnQh1HF7RYoZ4rpQWWFc3PoTu=9m5w@mail.gmail.com>
In-Reply-To: <xmqqlh5rfd0t.fsf@gitster.mtv.corp.google.com>

On Thu, Mar 10, 2016 at 6:27 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

> 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.

We could revert these local var rename patches at the end of the conversion, I think.

Show 8 quoted lines

> 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.

Yeah, I wish we have a semantic parser that can help with this sort of refactoring (can sparse do that?). Once we get AST tree, it should be easy to identify variable scope. Without it, maybe we can still rely on the compiler by renaming _both_ global and local vars to something else. That forces us (both patch writer and reviewers) to catch and examine every reference.

-- 
Duy