Christian Couder [off-list ref] writes:
Signed-off-by: Christian Couder <redacted>
---
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.
On Thu, Mar 10, 2016 at 6:27 AM, Junio C Hamano [off-list ref] wrote:
Christian Couder [off-list ref] writes:
quoted
Signed-off-by: Christian Couder <redacted>
---
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.
--
Duy
On Thu, Mar 10, 2016 at 1:54 AM, Duy Nguyen [off-list ref] wrote:
On Thu, Mar 10, 2016 at 6:27 AM, Junio C Hamano [off-list ref] wrote:
quoted
Christian Couder [off-list ref] writes:
quoted
Signed-off-by: Christian Couder <redacted>
---
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.
quoted
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.