On 28 June 2018 at 10:12, Pekka Paalanen <ppaala...@gmail.com> wrote: > On Wed, 27 Jun 2018 17:49:34 +0100 > Emil Velikov <emil.l.veli...@gmail.com> wrote: > >> Hi Pekka, >> >> A couple small ideas come to mind: >> >> On 27 June 2018 at 14:47, Pekka Paalanen <ppaala...@gmail.com> wrote: >> > From: Pekka Paalanen <pekka.paala...@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 <dani...@collabora.com> >> > Signed-off-by: Pekka Paalanen <pekka.paala...@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 _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel