[haiku-commits] Re: haiku: hrev52133 - src/apps/mediaconverter
- From: Adrien Destugues <pulkomandy@xxxxxxxxxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Wed, 25 Jul 2018 18:37:57 +0200
On Wed, Jul 25, 2018 at 06:06:48PM +0200, Dario Casalinuovo wrote:
So, again, this code is way out of the standard quality you claim publicly
to respect. Some patches of mine were rejected for minimal (but right ones
I don't say the contrary) changes. So I expect people to revert and redo
the work as I always did. Otherwise, I will do it myself.
I agree it is not correct and I already complained in other places
about code being merged from Gerrit too fast, it's not because the
"merge" button is easier to reach that we should press it without
careful review.
If you don't agree about a change, give a -2 on Gerrit to prevent it from
being merged, rather than complaining on the mailing list after the fact.
In any case I expect people to review correctly, in this case it seems 3
devs accepted code they didn't understand.
Using Clear instead of memset seems fine, so that particular patch is
right in the current context. How we implement Clear - or wether it is
needed at all - should have been fixed earlier already.
The previous patch where the problem lies is
https://review.haiku-os.org/#/c/haiku/+/215/ . I simply missed the fact
that Clear() was just a memset, which of course still isn't appropriate
even if hidden in a method. No one is perfect, and there were just 2
devs (not 3) reviewing it.
We should spend more time on getting Gerrit to send mail and/or IRC
notifications when something happens, as it would allow more people to
review things. I hope to see your -2s on Gerrit then, which is the
better place to do it.
--
Adrien.
Other related posts:
