From owner-dev-commits-src-all@freebsd.org Tue Feb 9 18:46:05 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 20D09547445; Tue, 9 Feb 2021 18:46:05 +0000 (UTC) (envelope-from carpeddiem@gmail.com) Received: from mail-il1-f173.google.com (mail-il1-f173.google.com [209.85.166.173]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DZsKn08frz3QRy; Tue, 9 Feb 2021 18:46:04 +0000 (UTC) (envelope-from carpeddiem@gmail.com) Received: by mail-il1-f173.google.com with SMTP id q5so17042970ilc.10; Tue, 09 Feb 2021 10:46:04 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=JgadIckbR1xbVO2/hbazlRYqcIbzG2YUnVP0/xZ1qOU=; b=mDy4TSpgH/KuAxp5L1uj8tW62XOwGyGuRmptVYu/I3EIBrD92HtAYlDepYzDc7eTQj mxazAgAHmHa1xpR6nfmUgZ9x+KVZoOIdHyA+i47kuOH/Qnkp3wr9HsECedEqXyTav3MY xfX72h9QqCthtvbEBCWDTVbIXRcmpS7P1nYgaHWay6ANyRKsdXdfNn8ONLKcQiYPNQpr +EDZfzw4M3UPeJhr76L1mSiYhii+RKn4AauY4xryfHL8KH2lQic0Sp5sPDvRGzW4OTIU qVdIrRYLzaWNxBF2opoGTs5x+LF1IDEhbWtY+HSYeedB2BQ7i9hFE5GMi7oHR84bN4g9 tvsQ== X-Gm-Message-State: AOAM533Hq7IPyHHWmhT5D0B6y6Wq5rkGYVGcjVtHsV26UbGVcs/ORUHb 6vOZGaJYNJOZSzIIOlxIJRNXFsDIeUWJr2z2xfs= X-Google-Smtp-Source: ABdhPJx4ckHewkwtFzVEMnTljshhjrUGJYEVabledpBhsI+6Rgj2akmBtamIYZ6fQrhukMdK+KKWH+9TuntDub1ymw8= X-Received: by 2002:a05:6e02:4cc:: with SMTP id f12mr21800495ils.182.1612896364070; Tue, 09 Feb 2021 10:46:04 -0800 (PST) MIME-Version: 1.0 References: <202102081915.118JFXkJ067892@gitrepo.freebsd.org> <3E64387A-42DD-4470-8893-5B774F19754E@freebsd.org> <8E61EA5C-39D1-49CC-8319-06E9192FF735@freebsd.org> In-Reply-To: From: Ed Maste Date: Tue, 9 Feb 2021 13:45:40 -0500 Message-ID: Subject: Re: git: af366d353b84 - main - amd64: implement strlen in assembly To: Mateusz Guzik Cc: Jessica Clarke , Kevin Bowling , "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 4DZsKn08frz3QRy X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Feb 2021 18:46:05 -0000 On Tue, 9 Feb 2021 at 08:41, Mateusz Guzik 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.