Chao Li <[email protected]> writes:
> The relevant code looks like this:
> msg_one = _("invalid page in block %u of relation \"%s\””);
> ereport(elevel,
> errcode(ERRCODE_DATA_CORRUPTED),
> errmsg_internal(msg_one, first + first_off, rpath.str) :
I agree that that isn't great code, but what I don't like about it
is the separation between where the format string is defined and
where it is used. It'd be very easy for the %-escapes to get out of
sync with the types of the actual parameters, and if they did, the
compiler would not warn you. I think we ought to try to recast this
into the normal usage pattern where the format is literal within the
errmsg call. I see the comment about avoiding code duplication, but
to my mind this is a terrible solution.
> errmsg_internal() is explicitly intended for non-translatable, internal
> messages. Passing an already translated string to it feels inconsistent with
> its documented purpose.
[ shrug... ] We do that in some other places, I believe, the idea
being just to not waste cycles by passing a message through
translation twice. (In theory perhaps you could even end with the
wrong message, though that'd take a highly-unlikely match of one
translated message to some other untranslated message.) I don't like
this code for the reason I gave above, but I don't think its use of
errmsg_internal is problematic per se. Maybe we should amend the
comment of errmsg_internal to mention such usage.
> This patch doesn't affect runtime behavior, but it avoids confusion for
> future readers, and helps prevent similar misuse elsewhere.
Well, actually, it breaks translation of these messages completely.
With this coding there is nothing that will cue xgettext to pick up
these strings as translatable. You could use gettext_noop() instead
of _() to do that, but that doesn't fix my beef about the separation
between format string and format arguments.
regards, tom lane
