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

From: Carlo Arenas <carenas@gmail.com>
Date: Sep 1, 2021, 18:03 UTC
Message-ID: <CAPUEspj2PNCjM2cLU6WzQ+rV7ftRBTfptKwht7N-=zVFTzan-A@mail.gmail.com>
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:

Show 5 quoted lines

>
> 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.

Show 6 quoted lines

> > 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

Show 9 quoted lines

> 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