On Thu, Mar 10, 2016 at 6:27 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Christian Couder <christian.couder@xxxxxxxxx> writes: > >> Signed-off-by: Christian Couder <chriscool@xxxxxxxxxxxxx> >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html