[PATCH wayland] contributing: review rules for bugs

[Emil Velikov]

On 28 June 2018 at 10:12, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Wed, 27 Jun 2018 17:49:34 +0100
> Emil Velikov <emil.l.velikov at gmail.com> wrote:
>
>> Hi Pekka,
>>
>> A couple small ideas come to mind:
>>
>> On 27 June 2018 at 14:47, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>> >
>> > Half of the ideas came from Daniel but most of them are reworded, the
>> > rest are my thoughts.
>> >
>> > Mention compiler warnings specifically, and be more explicit on what
>> > kind of code or bugs or bug fixes are acceptable or not. Clarify commit
>> > scope.
>> >
>> > Cc: Daniel Stone <daniels at collabora.com>
>> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>> > ---
>> >  CONTRIBUTING.md | 19 ++++++++++++++++---
>> >  1 file changed, 16 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
>> > index 70d0eca..51bef89 100644
>> > --- a/CONTRIBUTING.md
>> > +++ b/CONTRIBUTING.md
>> > @@ -223,11 +223,24 @@ include tests excercising the additions in the test suite.
>> >  - The code fits the existing software architecture, e.g. no layering
>> >  violations.
>> >
>> > -- The code is correct and does not ignore corner-cases.
>> > +- The code is correct and does not introduce new failures for existing users,
>> > +does not add new corner-case bugs, and does not introduce new compiler
>> > +warnings.
>> Compiler warnings vary greatly on the compiler and it's version. It
>> does become a nuisance when committer/reviewer is using a newer
>> version which flags dozens of warnings.
>
> Yes. There is no need to get out of one's way to hunt for possible
> warnings, but if someone does notice new warnings, they should be
> fixed. If a reviewer gets them but the author/submitter doesn't, the
> reviewer can help with them. It's common sense to me.
>
> I've been wondering if the CI build should actually turn -Werror on,
> but that's better to discuss after if/when we migrate to MR-based work
> flow.
>
>> That aside, worth loosening this for patches where existing code is
>> copied or moved?
>
> It is already loosened: they are not new warnings if they were there
> before. If code motion results in new warnings, they should ideally be
> fixed. If the fix makes the code motion harder to review, it's ok to
> fix it in another patch before or after.
>
> I think we also need to be picky on how much details we are going to
> write down here. It is very easy to extend the guidelines with details
> so far that it becomes too long to read. That is why I try to be as
> terse as possible.
>
> It's the spirit of the guidelines that matters, they are not to be
> interpreted by the letter, and there may always be special
> circumstances where the reasonable action is to overlook something.
> Even with the guidelines, we still rely very much on the individual
> judgement. The important thing is to get people to the right mindset in
> general.
>
> Perhaps the above would be worth a paragraph in CONTRUBUTING.md?
>
It was meant as food for thought, inspired by some long threads I've
seen where devs discuss various compiler warnings.
You're spot on being terse and providing a feel (or spirit as you call
it) is the key point.

Apologies for the distraction this has caused :-\

-Emil