On 2017-10-13 16:03, Jedrzej Nowacki wrote:
If you do like that then you are doing it wrong. Review process is
_not_ based
on a name / company / sun activity. It is based on the change content.
Even
best people do mistakes.
I'm entirely with Jedrzej here. Do not +2 if you don't understand the
change - all of it, regardless of who it's from. If you only did a
cursory review, give a +1, at most, and mention that it's incomplete. If
you do not understand the header change you used as an example, ask for
it to be clarified (in the commit message, preferably), since chances
are that other reviewers or later git history users might have the same
problems understanding code.
I would even qualify what Thiago wrote in response: I don't want
Thiago's +2 on a QStringView patch if he didn't understand the code.
Sure, I don't want unreasonable bikeshedding over subjective matters in
"my" code, since the author of the patch should have the last say in
these, but I don't want someone to usher my code through because I
happen to be the principal author of the code.
Another, slightly related aspect, and one of the reasons why I said
Viktor got it the wrong way around is that I prefer cross-company
reviews over intra-company ones, and so should anyone. If you ask a
member of your own organisation to review a patch, there are social
effects at work that will make the review less valueable than one done
by an outsider: unreasonable trust in your colleagues, trying to be
helpful by unblocking a patch, ...
Consequently, I accept a +2 from a KDABian only if I'm
above-the-ordinary-ly sure about the correctness of my code. Otherwise,
I wait for Thiago or Olivier or any@tqc to give a second opinion.
Yes, it's a pain to wait for code review, but office-neighbour reviews
just tend to produce bad code/apis, and often you can do better by
writing a page of commit message _more_ than you usually would to
anticipate and answer questions regarding the code ahead of time.
Thanks,
Marc
_______________________________________________
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development
