Date: Tue, 9 Feb 2021 13:45:40 -0500 From: Ed Maste <emaste@freebsd.org> To: Mateusz Guzik <mjguzik@gmail.com> Cc: Jessica Clarke <jrtc27@freebsd.org>, Kevin Bowling <kevin.bowling@kev009.com>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@freebsd.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@freebsd.org> Subject: Re: git: af366d353b84 - main - amd64: implement strlen in assembly Message-ID: <CAPyFy2AaoFZRoCELcfTmc6gSVF76%2BLdWgECBda4OXR13kN6Pwg@mail.gmail.com> In-Reply-To: <CAGudoHG%2BiKB4q=QoFEadtHsXy9ODX%2BkEHn-hU71F8yGEbzP5MA@mail.gmail.com> References: <202102081915.118JFXkJ067892@gitrepo.freebsd.org> <3E64387A-42DD-4470-8893-5B774F19754E@freebsd.org> <CAK7dMtA5rbT6QoCK2HQfzLY2f%2B3kbhdJp0ERmRpc_CVAog%2B6bg@mail.gmail.com> <8E61EA5C-39D1-49CC-8319-06E9192FF735@freebsd.org> <CAGudoHG%2BiKB4q=QoFEadtHsXy9ODX%2BkEHn-hU71F8yGEbzP5MA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 9 Feb 2021 at 08:41, Mateusz Guzik <mjguzik@gmail.com> wrote: > > Examples from recent past (all fixed): I don't think the examples are all good ones - several are failures of our tooling, not of code review. The limited review effort we have shouldn't be spent pointing out style(9) violations or build-breaking typos - our CI should be doing this. Reviewers should be able to expect that a patch builds and boots, and that it is sufficiently compliant with coding style, etc. I agree there's little value in putting changes into review solely because it's some sort of checklist item. That said, several of the reviews you identified had value in refining the change prior to commit, even if an issue escaped. > Would a review prevent the problem? That is plausible, but given track > record indicated above, I would not count on it. It wouldn't guarantee it, but the review could serve as a rendezvous point for testing - one of the examples even demonstrates that use. Of course we'd still need to get someone with an interest in the associated platform(s) to test, regardless of where/how the patch is conveyed. > And now we reach this change. > > I did the due diligence in testing (glibc, my test jig, actual > workloads). Because of this, while I can never claim the routine is > bug free, I doubt bugs (if any) would be found in a review and this is > why I did not ask for one. IMO back to (missing) tooling - eventually pre-commit CI build/test (whether integrated into Phabricator or elsewhere) should find these sorts of issues.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAPyFy2AaoFZRoCELcfTmc6gSVF76%2BLdWgECBda4OXR13kN6Pwg>