Re: [PATCH 08/11] xfs: widen ondisk timestamps to deal with y2038 problem

[Dave Chinner <david@fromorbit.com>]

On Thu, Aug 20, 2020 at 08:11:27AM +0300, Amir Goldstein wrote:
> On Thu, Aug 20, 2020 at 3:03 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> > On Wed, Aug 19, 2020 at 02:43:22PM -0700, Darrick J. Wong wrote:
> > So... while we could get rid of the union and hand-decode the timestamp
> > from a __be64 on legacy filesystems, I see the static checker complaints
> > as a second piece of evidence that this would be unnecessarily risky.
> >
> 
> And unnecessarily make the code less readable and harder to review.
> To what end? Dave writes:
> "I just didn't really like the way the code in the encode/decode
> helpers turned out..."
> 
> Cannot respond to that argument on a technical review.

Sure you can.

I gave a possible alternative implementation in my review, Darrick
pointed out it has problems and asked why I suggested a different
implementation. My answer was simply "I didn't really like the code,
maybe it could be done differently".

That's perfectly normal. If you -don't like the way the code is
written- then that should be a part of the review feedback. If
there's no other alternative to the way the code was presented, then
the reviewer is just going to have to live with it. That's perfectly
acceptible.

> I can only say that as a reviewer, the posted version was clear and easy
> for me to verify

Which is your personal opinion. Opinions differ and, again, there's
nothing wrong with that.

But you're stating that you don't want reviewers to express an
opinion on how the code looks because you "cannot respond to that on
a technical review". That's kind of naive - when was the last time
you asked someone to reformat the code because you found it was hard
to read?

We've always considered how the code looks as a key metric in
determining if the code will be maintainable. Why else would we care
about macro nesting, typedefs, keeping functions short and concise,
etc? That's all about how the code looks and how easy it is to read,
and hence we can infer the cost of maintainability of the code bein
proposed from that. That's all technical analysis of the code being
proposed, and it's all subjective.

See what I'm getting at here? Comments about the *look* of the code
is valid technical feedback, and we can argue *technically* at
length about whether the code is structured the best way it could be
done. And we often do this at length just because someone simply
"doesn't like the way the proposed code currently looks"...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com