On Tue, Feb 22 2022, Johannes Schindelin wrote:
> Hi Julia,
>
> I would like to loop you in here because you have helped us with
> Coccinelle questions in the past.
Thanks. Probably better to CC the relevant ML, adding it.
> On Mon, 21 Feb 2022, Abhradeep Chakraborty wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
>>
>> > That should be fairly easy to do though, and if not we could always
>> > just dump these to stderr or something if a
>> > git_env_bool("GIT_TEST_PARSE_OPTIONS_DUMP_FIELD_HELP", 0) was true,
>> > and do the testing itself in t0012-help.sh.
>>
>> Okay but if the logic can't be implented in the `parse-options.c` file
>> (most probably I will be able to implement the logic), would you allow
>> me to try the `coccinelle script` method you mentioned?
>
> The task at hand is to identify calls to the macro `OPT_CMDMODE()` (and
> other, similar macros) that get a fourth argument of the form
>
> N_("<some string>")
>
> The problem is to identify `<some string>` that ends in a `.` (which we
> want to avoid) or that starts with some prefix and a colon but follows
> with an upper-case character.
>
> In other words, we want to suggest replacing
>
> N_("log: Use something")
>
> or
>
> N_("log: use something.")
>
> by
>
> N_("log: use something")
>
> Ævar suggested that Coccinelle can do that. Could you give us a hand how
> this would be possible using `spatch`?
I probably shouldn't have mentioned that at all, and I think this is
academic in this context, because as noted we can just add this to
parse_options_check() (linking it here again for off-git-ml context):
https://lore.kernel.org/git/220221.86tucsb4oy.gmgdl@xxxxxxxxxxxxxxxxxxx/
We now pay that trivial runtime overhead every time, we could run it
only during the tests if we ever got worried about it.
And it's a lot less fragile and easy to understand than running
coccicheck, i.e. as nice as it is it's still takes a while to run, is
its own mini-language, needs to be kept in sync with code changes etc.
So by doing it at runtime we can adjust messages, code & tests in an
atomic patch more easily (i.e. not assume that you ran some cocci target
to validate it).
It also makes it really easy to do things that are really hard (or
impossible?) with coccinelle. I.e. some of these checks are run as a
function of what flag gets passed into some function later on, which in
the general case would require coccinelle to have some runtime emulator
for C code just to see *what* checks it wants to run.
That being said (and with the caveat that I've only looked at this code,
not done this myself) if you clone linux.git and browse through:
git grep -C100 -F coccilib.report '*.cocci'
You can see a lot of examples of using cocci for these sorts of checks.
And the same goes if you clone coccinelle.git and do:
git grep -C100 @script: -- tests
For linux.git it's documented here:
https://github.com/torvalds/linux/blob/master/Documentation/dev-tools/coccinelle.rst
I.e. it's basically writing the sort of cocci rules we have in-tree with
a callback script that complaints about the required change.
For our use it would probably better (in lieu of parse_options_check(),
which is the right thing here) to just have a normal *.cocci file and
complain if it applies changes. We already error in the CI if those need
to apply any changes.
But I don't off-hand know how to do that. E.g. I was trying the other
day to come up with some coccinelle rule that converted:
die("BUG: blah blah");
To:
BUG("blah blah");
And while I'm sure there's some way to do this, couldn't find a way to
write a rule to "reach in" to a constant string, apply some check or
search/replacement on it, and to do a subsequent transformation on it.
In this case the OPT_BLAH(1, 2, 3, "string here") has OPT_BLAH() as a
macro. I can't remember if there's extra caveats around using coccinelle
for macros v.s. symbols.
Disclaimer: By "couldn't" I mean I grepped the above examples for all of
a few minutes quickly skimmed the coccinelle docs, didn't find a
template I could copy, then ended up writing some nasty grep/xargs/perl
for-loop instead :)
