Skip to content

script_nullpo_end: Use error parameter as an argument to printf, rather than building part of the formatter

Free Yorp requested to merge strung-along-for-nearly-four-years into master

PR !270 (merged) is an ongoing investigation into outstanding compiler warnings.

One such warning is warning: format '%s' expects a matching 'char*' argument from script_nullpo_end(nd, STRPRINTF("no such npc: '%s'"_fmt, name)); in src/map/script-fun.cpp. No such warning is emitted from a similar line in src/map/atcommand.cpp: AString output = STRPRINTF("Jump to %s"_fmt, npc);.

script_nullpo_end is a macro, rather than a function. error is passed in verbatim by the preprocessor. The macro definition of script_nullpo_end includes lines such as the following:

PRINTF("script:%s: " #error " @ %s\n"_fmt, BUILTIN_NAME(), nullpo_nd->name);

#<variable> instructs the preprocessor to transform <variable> into a literal string. In this case, the string literal:

"STRPRINTF(\"no such npc: '%s'\"_fmt, name)"

So the entire line partly resolves to something like:

printf("script:%s: STRPRINTF(\"no such npc: '%s'\"_fmt, name) @ %s\n"_fmt, BUILTIN_NAME(), nullpo_nd->name);

Once the compiler sees this, it throws out the warning seen here, because printf is seeing three placeholders in the formatter, but is only given two value arguments.

Checking with gcc -E, the full expansion is as follows:

    if (nullpo_chk("script-fun.cpp", 4885, __PRETTY_FUNCTION__, nd)) { if (st->oid) { dumb_ptr<npc_data> nullpo_nd = map_id_is_npc(st->oid); if (nullpo_nd && nullpo_nd->name) { ({ struct format_impl { constexpr static FormatString print_format() __asm__("_print_format") { return "script:%s: " "STRPRINTF(\"no such npc: '%s'\"_fmt, npc)" " @ %s\n"_fmt; } }; cxxstdio::PrintFormatter<format_impl>::print((
# 4885 "script-fun.cpp" 3 4
   stdout
# 4885 "script-fun.cpp"
   ), (builtin_functions[st->stack->stack_datav[st->start].get_if<ScriptDataFuncRef>()->numi].name), nullpo_nd->name); }); } else if (nullpo_nd) { ({ struct format_impl { constexpr static FormatString print_format() __asm__("_print_format") { return "script:%s: " "STRPRINTF(\"no such npc: '%s'\"_fmt, npc)" " (unnamed npc)\n"_fmt; } }; cxxstdio::PrintFormatter<format_impl>::print((
# 4885 "script-fun.cpp" 3 4
   stdout
# 4885 "script-fun.cpp"
   ), (builtin_functions[st->stack->stack_datav[st->start].get_if<ScriptDataFuncRef>()->numi].name)); }); } else { ({ struct format_impl { constexpr static FormatString print_format() __asm__("_print_format") { return "script:%s: " "STRPRINTF(\"no such npc: '%s'\"_fmt, npc)" " (no npc)\n"_fmt; } }; cxxstdio::PrintFormatter<format_impl>::print((
# 4885 "script-fun.cpp" 3 4
   stdout
# 4885 "script-fun.cpp"
   ), (builtin_functions[st->stack->stack_datav[st->start].get_if<ScriptDataFuncRef>()->numi].name)); }); } } else { ({ struct format_impl { constexpr static FormatString print_format() __asm__("_print_format") { return "script:%s: " "STRPRINTF(\"no such npc: '%s'\"_fmt, npc)" " (no npc)\n"_fmt; } }; cxxstdio::PrintFormatter<format_impl>::print((
# 4885 "script-fun.cpp" 3 4
   stdout
# 4885 "script-fun.cpp"
   ), (builtin_functions[st->stack->stack_datav[st->start].get_if<ScriptDataFuncRef>()->numi].name)); }); } st->state = ScriptEndState::END; return; };

Note the string literal: "STRPRINTF(\"no such npc: '%s'\"_fmt, npc)". script_nullpo_end currently can't take as its argument for error any expression which, when stringified, contains something which printf will interpret as its formatter expecting another argument.

This appears to have been broken since it was first introduced in 594eafd4 since the first revision implicitly had these requirements, and also introduced usage which didn't meet them.

This rewrites each PRINTF line in script_nullpo_end from the template

PRINTF("script:%s: " #error " @ %s\n"_fmt, BUILTIN_NAME(), nullpo_nd->name);

to something like

PRINTF("script:%s: %s @ %s\n"_fmt, BUILTIN_NAME(), error, nullpo_nd->name);

This means that instead of an expression being stringified, error is interpreted as an expression that resolves (or decays) to a string.

This seems consistent with all current usage of script_nullpo_end.

Merge request reports