Re: [PATCH v1 3/4] advice: remove uses of global `advice_` variables

On Tue, Aug 03, 2021 at 00:06:48 +0200, Johannes Schindelin wrote:

On Fri, 30 Jul 2021, Ben Boeckel wrote:

quoted

There are now function APIs to access this information, so the global
variables are no longer needed to communicate their values.

This commit message implies that the reader remembers that `hw/advise-ng`
introduced a new advice API with the express intent to eventually replace
those global `advice_*` variables.

However, it is not the responsibility of the reader to remember that. It
is the responsibility of the commit message to describe this (so that the
reader can either remember it, or learn about it in the first place).

That makes sense, thanks. I guess I'm used to projects where I'm at
least peripherally aware of most of what's going on, but that's because
I work on cross-cutting concerns on them for the most part (build
systems, CI, software process).

Now, as for the diff, I can guess just how tedious all of the
semi-repetitive `advice_*` -> `advice_enabled(ADVICE_*)` changes were from
trying to verify that they are all correct.

One of the times a case-insensitive word diff rendering would be handy.
(Then letting the compiler verify that the new side actually works.)

Big thank you for that!

You can thank this Vim macro which made the tedious bits trivial:

    iadvice_enabled(<Esc>wgUwea)<Esc>]q

However, even reading through the diff the second time, I managed to miss
the subtlety that there were two `advice_set()` calls strewn in.

As I pointed out in my review of patch 1/4, I would much prefer to have
the addition of those callers in 1/4 along with the introduction of said
function.

However, now that I write this, I would like to correct my advice (pun
intended) from 1/4 to leave the removal of the assignment of the global
`advice_*` variables here: It would make much more sense to move them to
patch 4/4.

Sounds good. I'll still keep it as a separate patch, but just have one
for the read-only side, then a new patch which adds the write API and
transforms the 2 write instances. The final patch can then stay the
same. In short:

  - 2/4 (read-only bits) -> v2 1/4
  - 3/4 -> v2 2/4
  - 1/4 + 3/4 (write bits) -> v2 3/4
  - 4/4 -> mostly the same

Thanks,

--Ben