| Message ID | 20210901091941.34886-1-carenas@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | developer: support pedantic | expand |
On Wed, Sep 01, 2021 at 02:19:37AM -0700, Carlo Marcelo Arenas Belón wrote: > first patch was suggested[1] by Peff, so hopefully my commit message > and his assumed SoB are still worth not mixing it with patch 2 (which > has a slight different but related focus and touches the same files) > but since it is no longer a single patch, lets go wild. My SoB is fine there (though really Ævar did the actual thinking; I just deleted a lot of lines in vim :) ). Patch 2 looks good to me, though I kind of wonder if it is even worth having an option to turn it off. > patches 3 and 4 are optional and mostly for RFC, so that a solution > to any possible issue that the retiring of USE_PARENS_AROUND_GETTEXT_N > are addressed. 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. -Peff
[I should have included this in my just-sent [1], but forgot] On Wed, Sep 01 2021, Jeff King wrote: > On Wed, Sep 01, 2021 at 02:19:37AM -0700, Carlo Marcelo Arenas Belón wrote: > >> first patch was suggested[1] by Peff, so hopefully my commit message >> and his assumed SoB are still worth not mixing it with patch 2 (which >> has a slight different but related focus and touches the same files) >> but since it is no longer a single patch, lets go wild. > > My SoB is fine there (though really Ævar did the actual thinking; I just > deleted a lot of lines in vim :) ). > > Patch 2 looks good to me, though I kind of wonder if it is even worth > having an option to turn it off. > >> patches 3 and 4 are optional and mostly for RFC, so that a solution >> to any possible issue that the retiring of USE_PARENS_AROUND_GETTEXT_N >> are addressed. > > 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. Agreed. Per the rationale in my version of the commit messsage for Carlo's 1/4 at [1] I don't think this was ever worth it. I.e. it wasn't even an N_()-specific issue to begin with, but just a migration from usage() (takes a string) to usage_with_options() (takes an array of strings). I just submitted a related series at [2] to fix the alignment of continued strings containing "\n" in parse-options.c, which is the reason we need to support "\n"-continued strings at all in parse-options.c. 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 needed 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]). 1. https://lore.kernel.org/git/patch-1.1-d24f1df5d49-20210901T112248Z-avarab@gmail.com 2. https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@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