Re: [RFC PATCH v2 0/4] developer: support pedantic

From: Carlo Arenas <carenas@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Jeff King <peff@peff.net>,
	git@vger.kernel.org, phillip.wood@dunelm.org.uk,
	gitster@pobox.com, mackyle@gmail.com, sunshine@sunshineco.com
Subject: Re: [RFC PATCH v2 0/4] developer: support pedantic
Date: Wed, 1 Sep 2021 11:03:02 -0700	[thread overview]
Message-ID: <CAPUEspj2PNCjM2cLU6WzQ+rV7ftRBTfptKwht7N-=zVFTzan-A@mail.gmail.com> (raw)
In-Reply-To: <878s0gwmvq.fsf@evledraar.gmail.com>

On Wed, Sep 1, 2021 at 4:34 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> On Wed, Sep 01 2021, Jeff King wrote:
>
> > Patch 2 looks good to me, though I kind of wonder if it is even worth
> > having an option to turn it off.

I failed to mention "Patch 2" isn't ready as it will break the mingw64
builds in Windows.

While I also hope there is no need to have an option to turn it off,
realistically I expect the wall of errors is still there for non
gcc/clang compilers and I am curious if some developer still using
RHEL 7 (or a clone) will report back and will be forced to use it.

> > IMHO the issue it is trying to find is not worth the inevitable problems
> > that hacky perl parsing of C will cause (both false positives and
> > negatives). Not a statement on your perl code, but just based on
> > previous experience.
> >
> > So I'd probably take the first two patches, and leave the others.

Maybe better to discard the whole series and rebase it on top of Ævar's then

> So I think (per [1]) that we should just remove
> USE_PARENS_AROUND_GETTEXT_N, and that the 3/4 here isn't needed at all
> (aside from concerns about parsing C with Perl).
>
> But in the future we need any assertion for this sort of thing at all
> it would be better built on top of my [2]. I.e. parse-options.c could do
> some basic sanity checking on the usage array it takes, we'd then end up
> detecting the issue USE_PARENS_AROUND_GETTEXT_N was trying to address,
> and more (such as the alignment problems I fixed in 1/2 of my [2]).

Regardless of how ugly my perl script was, I don't think this specific
issue could be
handled by the C code, as it needs to be done with preprocessed sources.

note also, it is not really parsing C, but just looking at a regex
which could have been as well handled with a simple grep.

The script was built under the incorrect assumption it would be useful
to track exceptions and have a way to keep that state (as well as the
code) cleanly out of the way (which is why patch 4 is also there).

Carlo