Re: [PATCH v5 10/13] xdiff: delete rchg aliasing
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 25, 2025, 07:09 UTC
- Message-ID
- <xmqqh5wrhutd.fsf@gitster.g>
- In-Reply-To
- <CAH=ZcbAm8++qGNX883epk0h3NA-Z=8+RX1bDJeTe=n2jjSsdmg@mail.gmail.com>
Ezekiel Newren <ezekielnewren@gmail.com> writes:
Show 8 quoted lines
> 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.
Show 8 quoted lines
>> 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.
Show 6 quoted lines
> 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.