On 2024-03-19 01:32, Junio C Hamano wrote:
Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
It's entirely subjective, of course, so no right-or-wrong answer, but
I personally do not find that this change improves code quality or
readability.
I agree that this is entirely subjective. To those who wrote these
variable decls and inits, what they wrote was the most readable,
wasn't it? It probably falls into the "to some readers the existing
code may not be perfect, but once it is written, it is not worth a
patch noise to fix it" category.
There's no doubt that it was the most readable form to the people who
wrote the code, which was some time ago, but the time inevitably passes
and the surrounding code changes over time, maybe even some new coding
guidelines become introduced, etc.
Things inevitably change, that's all I'm trying to say.
With my reviewer hat on, I spent an inordinate amount of time staring
at this change trying to locate each variable's new location to verify
that no initializers were dropped and that the declared type hadn't
changed.
It is true that "cleaning up, no behaviour changes intended" patches
are unpleasant to review. They are boring to read, and the risk of
breakage due to mistake is unnecessary and severe.
But if the result is objectively better, such a one-time cost may be
worth it. We are investing into the better future. For example, we
may have an unsorted mess of an enum definition, and we do
appreciate in the longer run, such a definition were "more or less"
sorted within the constraint of some other criteria (like, "errors
get negative value"). If the enum is a huge one, it may need some
careful reviewing to verify such a change that turns the unsorted
mess into a sorted nice list, but the cost of doing so may be
justified.
I fully agree that reviewing code-cleanup patches it usually boring
and often taxing. I mean, why change something that has served us
well for years, just to make it look nicer in someone's eyes? What
does even "nicer" mean to everyone?
Well, I sometimes look at the code as if it were a beautiful painting.
To some people, it doesn't matter if a painting has some rough areas,
as long as it can be hung on a wall. To them, it's just a square thing.
Though, to some other people, spotting such rough areas is what makes
the painting less beautiful to them. Paintings usually cannot be fixed
easily, but fixing the code is much easier.
Of course, "nicer" is very hard to define, but I believe that, in the
domain of computing, it could be partially defined as "more consistent
and flowing better". That's what I tried to achieve with this patch,
and I hope I've managed to convey my viewpoint.
Does the change in this patch qualify as "objectively better"? I
dunno.
I'd say that these changes qualify as "semi-objectively nicer", but
surely not as "objectively better". For any changes to be objectively
better, they'd need to introduce some functional changes to the code,
while a well-performed code cleanup actually introduces no functional
changes.
