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@xxxxxxxxxx> 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@xxxxxxxxxxxxx
