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

From: Christian Couder <christian.couder@gmail.com>
Date: Mar 10, 2016, 10:45 UTC
Message-ID: <CAP8UFD0UYsaz6ZatZCXuxmCc-ge_Tu3mL16Bz25EUFGf9ma1Ow@mail.gmail.com>
In-Reply-To: <CACsJy8Boz1XLv7yh5GsOQnQh1HF7RYoZ4rpQWWFc3PoTu=9m5w@mail.gmail.com>

On Thu, Mar 10, 2016 at 1:54 AM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 33 quoted lines

> On Thu, Mar 10, 2016 at 6:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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.
>
>> 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.

I agree that the change in the patch is not very nice. I can do one of the following:

1) do as Junio suggests, just discard this change and wait until
p_value is moved into apply_state for the problem to be solved; a
small downside of this is that if you compile with -Wshadow after each
change you will get some warnings until p_value is moved into
apply_state; I could move the patch to move p_value at the beginning
of the series but it is a bit involved and easier to review if it is
done towards the end,

2) revert these local var rename patches at the end of the conversion,
as Duy suggests

3) instead of renaming the local "p_value" variable, rename the global
"p_value" to maybe "state_p_value", so that when I move it into
apply_state I just need to s/state_p_value/state->p_value/.

I'd rather do 3) than 1) or 2) but I am also ok with 1) and 2).

Thanks, Christian.