[PATCH weston 1/5] gl-renderer: Rename gl_renderer_create to gl_renderer_display_create

[Daniel Stone]

Hi,

On 18 May 2016 at 15:25, Derek Foreman <derekf at osg.samsung.com> wrote:
> On 18/05/16 08:41 AM, Mike Blumenkrantz wrote:
>> In fairness, we'd likely be less short on review bandwidth if the
>> majority of that bandwidth was not in use to make/revise trivial
>> criticisms such as whitespace usage and comment grammar which are
>> guaranteed to be cleaned up in future patches; this leads to burnout on
>> both the code-writing side as well as the reviewing side since everyone
>> has become hyper attuned to the insignificant and non-functional
>> minutiae of patches rather than focusing more on expediting the
>> technical development of the protocol.
>
> Fair points, though I'm not certain "will certainly get fixed up later"
> is a given.  Certainly indenting and basic style is a mechanical problem
> that could be tested pre-commit hooks, and there should be no reason to
> bike shed that on the list at all.
>
> Grammar probably needs more serious consideration for protocol doc than
> elsewhere due to its potential impact on compositor implementors - but
> ever there probably not to the degree we're seeing lately.
>
> Follow up commits that do nothing but change style and grammar can make
> "git blame" less useful (when trying to figure out who would best review
> a piece of code - not just "arrrrgh who wrote this stupid bug") and
> provide churn for very little benefit, imho.

Yeah, I agree. I get that the bikeshedding can be annoying; I do (for
that reason, if no other) like tagging commits as 'RFC' or similar,
which is effectively, 'please just check out the technical concept and
don't worry about memory leaks or spelling mistakes right now'. But
given that it's pretty trivial to fix up, and you're likely to have to
rebase _anyway_, I don't see the harm in doing one round of review for
clarity.

Generally, there's no need to send out a subsequent revision round
just because someone has noted some typos - send it again if you need
a resend anyway to get people to pick it up after a rebase, or if
there have been notable changes, but you shouldn't be arriving at v17
just because you have difficulty spelling.

Similarly, 'no, I disagree' is a reasonable response to someone
bikeshedding your exact choice of variables or naming. Review is meant
to be a discussion, not something you just have to unilaterally
acqiuesce to.

> While we're drifting just slightly off topic here, I'm also concerned
> about the basic usage of some of our tags:
>
> Reviewed-by:  indicates rigorous technical review *AND* a firm
> conviction that the feature is important and should be included.
>
> Acked-by: Indicates a firm conviction that the feature is important and
> should be included, but no rigorous review has taken place.
>
> Signed-off-by: Indicates an assumption of responsibility for the code.
>
> I've seen a lot of Reviewed-by that I think is just "I looked at the
> spelling and you're good to go".  I'm starting to find this disconcerting.

Yes, completely agreed. There's a huge gulf between 'I looked at this
and nothing bad jumped out', versus 'I understand the implications
with this and am prepared to put my name down in agreeance with it
being a good idea'. I am pretty bad with review latency in general,
but on the other hand I do at least give them a reasonably thorough
look over ...

Cheers,
Daniel