On Fri, Nov 28, 2025 at 3:27 PM Tom Lane <[email protected]> wrote: > > Peter Smith <[email protected]> writes: > > On Fri, Nov 28, 2025 at 1:49 PM Tom Lane <[email protected]> wrote: > >> ... and, probably, less ability of the compiler to verify that the > >> variadic arguments match the format string. I think you've taken > >> this a bit too far. > > > * Or is it the use of the ternary operator to select the format > > string? If that's the issue, please note that my patch only introduced > > the ternary operator for the first two code fragments. The third > > fragment already uses ternaries in the same way on master, so I > > understood that to be an established pattern as well. > > Sadly, you were copying bad code that we need to fix. > > > I'd like to make sure I understand your concern correctly so I can > > revise the patch appropriately. > > My concern is that we follow a coding style that the compiler can > check. Most C compilers can only verify that printf-like format > strings match the actual arguments if the format string is a constant. > So if I fat-finger the format string in this example: > > --- a/src/backend/replication/logical/conflict.c > +++ b/src/backend/replication/logical/conflict.c > @@ -383,7 +383,7 @@ build_tuple_value_details(EState *estate, ResultRelInfo > *relinfo, > if (tuple_value.len > 0) > { > appendStringInfoString(&tuple_value, "; "); > - appendStringInfo(&tuple_value, _("existing local row %s"), > + appendStringInfo(&tuple_value, _("existing local row %d"), > desc); > } > > I'll hear about it: > > conflict.c: In function 'build_tuple_value_details': > conflict.c:386:38: warning: format '%d' expects argument of type 'int', but > argument 3 has type 'char *' [-Wformat=] > appendStringInfo(&tuple_value, _("existing local row %d"), > ^~~~~~~~~~~~~~~~~~~~~~~ > conflict.c:386:36: note: in expansion of macro '_' > appendStringInfo(&tuple_value, _("existing local row %d"), > ^ > > But I don't trust the compiler to see through a ternary expression > and check (both!) format strings against the actuals. > > In a quick test, the gcc version I have handy does seem to be able to > do that, but I don't think we should rely on that. Format strings > are something that is way too easy to get wrong, and most of us just > expect the compiler to cross-check them for us, so coding patterns > that might silently disable such compiler warnings are best avoided. > > (There's also some magic going on here to allow the compiler to see > through gettext(), but it's quite magic. We don't need to assume > multiple layers of magic will work reliably.) >
Got it. Thanks for the detailed reason. Here is a revised patch v2 that removes all ternaries from the format string decision making (including the ones that were already in master). ====== Kind Regards, Peter Smith. Fujitsu Australia
v2-0001-Simplify-format-strings-and-remove-ternaries.patch
Description: Binary data
