Thread: Header / Trailer Comment Typos for M4 macros
Hi hackers, While poking at the build system I stumbled upon some trivial trailer comment inconsistencies in config/c-compiler.m4. They can be fixed either way: either changing the macro names or changing the comment. PFA a patch that keeps the macro names. In hindsight though, it seems that PGAC_PRINTF_ARCHETYPE was meant to be PGAC_C_PRINTF_ARCHETYPE, judging by a half-second squint of surrounding, similar macros. Thoughts? Also in hindsight: it seems that, as suggested in the trailer typo, PGAC_PROG_CXX_VAR_OPT (a la the C version PGAC_PROG_CC_VAR_OPT) would be a good addition if we ever want to add the negative warning flags (for starter, Wno-unused-command-line-argument for clang++) to CXXFLAGS, but I assume it wasn't there in the final patch because we didn't use it (presumably because the patch was minimized?). Thoughts? Cheers, Jesse
Attachment
Jesse Zhang <sbjesse@gmail.com> writes: > While poking at the build system I stumbled upon some trivial trailer > comment inconsistencies in config/c-compiler.m4. They can be fixed > either way: either changing the macro names or changing the comment. PFA > a patch that keeps the macro names. Pushed, thanks. > In hindsight though, it seems that PGAC_PRINTF_ARCHETYPE was meant to be > PGAC_C_PRINTF_ARCHETYPE, judging by a half-second squint of surrounding, > similar macros. Thoughts? Maybe, but I doubt it's worth renaming. > Also in hindsight: it seems that, as suggested in the trailer typo, > PGAC_PROG_CXX_VAR_OPT (a la the C version PGAC_PROG_CC_VAR_OPT) would be > a good addition if we ever want to add the negative warning flags (for > starter, Wno-unused-command-line-argument for clang++) to CXXFLAGS, but > I assume it wasn't there in the final patch because we didn't use it > (presumably because the patch was minimized?). Thoughts? I'd be inclined not to add it till we have an actual use for it. Dead code tends to break silently. regards, tom lane
On Wed, Apr 22, 2020 at 12:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Jesse Zhang <sbjesse@gmail.com> writes: > > either way: either changing the macro names or changing the comment. PFA > > a patch that keeps the macro names. > > Pushed, thanks. > Thanks! > > Also in hindsight: it seems that, as suggested in the trailer typo, > > PGAC_PROG_CXX_VAR_OPT (a la the C version PGAC_PROG_CC_VAR_OPT) would be > > a good addition if we ever want to add the negative warning flags (for > > starter, Wno-unused-command-line-argument for clang++) to CXXFLAGS, but > > I assume it wasn't there in the final patch because we didn't use it > > (presumably because the patch was minimized?). Thoughts? > > I'd be inclined not to add it till we have an actual use for it. > Dead code tends to break silently. > For sure. I feel the same about dead code. I didn't make my question clear though: I'm curious what motivated the original addition of -Wno-unused-command-line-argument in commit 73b416b2e412, and how that problem did't quite manifest itself with Clang++. The commit mentioned pthread flags, but I tried taking out Wno-unused-command-line-argument from configure.in and it produces no warnings on my laptop (I know, that's a bad excuse). For context, I'm running Clang 11 on Ubuntu amd64. Cheers, Jesse
Jesse Zhang <sbjesse@gmail.com> writes: > I didn't make my question clear though: I'm curious what motivated the > original addition of -Wno-unused-command-line-argument in commit > 73b416b2e412, and how that problem did't quite manifest itself with Clang++. We didn't then have the convention of mentioning relevant mailing list threads in the commit log, but some excavation finds https://www.postgresql.org/message-id/flat/CALkS6B8Ei3yffHTnUsAovCPmO9kPTpgCArwyod7Ju2eWBm6%3DBA%40mail.gmail.com So it seems to have been specific to clang circa version 6.0. Maybe the clang boys thought better of this behavior more recently? [ experiments... ] I see no warnings on current macOS (Apple clang version 11.0.3) after removing the switch. So I guess they did fix it. We're pretty conservative about dropping support for old toolchains, though, so I doubt that we'd want to remove this configure check. Especially if we don't know how long ago clang changed it. regards, tom lane