Re: [PATCH v5 10/13] xdiff: delete rchg aliasing

[Junio C Hamano <gitster@pobox.com>]

Ezekiel Newren <ezekielnewren@gmail.com> writes:

> I'd agree that my reasons for making this change are insufficient. I
> think usage tracking tools _is_ a weak argument, but perhaps not quite
> as weak as what you're thinking. For example, when I renamed the rchg
> field to changed, it was as simple as right-clicking the field,
> choosing Rename, typing 'changed', and letting the IDE update every
> use. Patch 11/13, "xdiff: rename rchg -> changed in xdfile_t", was
> generated directly from that one action. That patch was clean because
> I had already gone through and removed all the aliases of that field.

If I am reading you correctly, you are describing IDE's syntax-aware
editor's symbol renaming feature; I am not quite sure what it has to
do with "usage tracking", which would be more of static analysis
thing, no?

Surely, IDE makes these symbol renaming easy and that would be one
reason that makes "because we will change this part of the code in
later commit" less relevant, isn't it?  Whether the struct member
rchg is accessed directly in the conditional and loop, or is used as
the source of an assignment to a local variable, it needs to be
renamed either way.  And with tools, it is not as bad.

>> Sure.  But looking at the use of rchg[12] in xdl_build_script(), if
>> they were "const char *", combined with the fact that they are local
>> and their addresses are never taken (to be leaked to our callers),
>> you wouldn't have much trouble with the current code, or would you
>> still have issues?
>
> For xdl_build_script() specifically it would work just fine keeping
> the local variable aliasing in.

And transliterating that directly to Rust would not cause the borrow
issue as you described?  Then it would be great.  One less thing to
worry about when we need to look at C and then write an equivalent
in Rust.

> 2. Keep this commit with reasons like this:
>   * Refactor churn: Later commits will refactor rchg.
>   * No additional meaning: The local variables express the same
> meaning as the struct field itself. Also, the conditional and the
> inner loop is easy enough to follow without using the local aliases to
> make the code shorter.

If you have to go route #2, I can live with it, but "No additional
meaning" is _not_ a valid reason to remove aliasing variables.

By definition, a local variable that aliases something like a deeply
nested structure member should *not* introduce any additional
meaning (in other words, if the code modifies that local variable
making it out of sync with the underlying structure, the variable is
not without "addtional meaning" and is no longer an alias).

The whole reason I asked you to justify removal of the local
variable on the basis of lack of readability improvement ("the
original is simple enough to read without shorter variables") was
just that.  "This variable is merely an alias to something else",
aka "There is no meaning added by the presence of this variable", is
*not* a valid reason to remove it by itself.

So, with the same code but with a better justification like "the
original uses a few local variables to shorten the code, but open
coding the access to underlying members of nested structure without
these local variables is not all that hard to read, so let's do so",
would probably be an acceptable explanation with no need for other
excuses, I would think.

Thanks.