Re: [PATCH v2 0/4] test-tool: split up "read-cache" tool

From: Taylor Blau <me@ttaylorr.com>
Date: Aug 24, 2021, 22:19 UTC
Message-ID: <YSVwioizjGKVSXQA@nand.local>
In-Reply-To: <cover-v2-0.4-00000000000-20210824T091204Z-avarab@gmail.com>

On Tue, Aug 24, 2021 at 11:15:21AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 11 quoted lines

> A re-roll addressing the feedback v1 got, see
> https://lore.kernel.org/git/cover-0.4-0000000000-20210607T115454Z-avarab@gmail.com
>
> I think the gist of that feedback from Emily was that v1 could be
> understood as aiming to optimize the perf test, but the main reason
> I'm doing this split up is to make the code easier to read. The
> changed commit message summarizes both goals.
>
> I also changed some nits in the code I spotted myself, e.g. "argc > 0"
> checks to just "argc", and a simpler way of providing the "cnt"
> default.

Playing devil's advocate for a moment: I think that the current implementation is somewhat easier to read as it lives in a single file, and makes clear that each option implies different behavior through the body of the loop.

If we're looking for things to clean up, I do like the conversion to the parse-options API instead of reading argv ourselves, but probably otherwise prefer the code as-is instead of split across many files.

But I may be in the minority, and there may be others who do find the split-up version easier to grok.

Thanks, Taylor