On 10/4/18 9:00 AM, David Malcolm wrote:
> -Wformat in the C++ FE doesn't work as well as it could:
> (a) it doesn't report precise locations within the string literal, and
> (b) it doesn't underline arguments for those arguments !CAN_HAVE_LOCATION_P,
> despite having location wrapper nodes.
>
> For example:
>
> Wformat-ranges.C:32:10: warning: format '%s' expects argument of type
> 'char*', but argument 2 has type 'int' [-Wformat=]
> 32 | printf("hello %s", 42);
> | ^~~~~~~~~~
>
> (a) is due to not wiring up the langhook for extracting substring
> locations.
>
> This patch uses the one in c-family; it also fixes string literal
> parsing so that it records string concatenations (needed for
> extracting substring locations from concatenated strings).
>
> (b) is due to the call to maybe_constant_value here:
> fargs[j] = maybe_constant_value (argarray[j]);
> within build_over_call.
>
> The patch fixes this by building a vec of location_t values when
> calling check_function_arguments.
> I attempted to eliminate the maybe_constant_value call here, but
> it's needed by e.g. check_function_sentinel for detecting NULL,
> and that code is in "c-family", so it can't simply call into
> maybe_constant_value (which is in "cp").
>
> With this patch, the output for the above example is improved to:
>
> Wformat-ranges.C:32:18: warning: format '%s' expects argument of type
> 'char*', but argument 2 has type 'int' [-Wformat=]
> 32 | printf("hello %s", 42);
> | ~^ ~~
> | | |
> | | int
> | char*
> | %d
>
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu (on top of
> the multiline.exp patch).
>
> OK for trunk?
>
> gcc/cp/ChangeLog:
> PR c++/56856
> * call.c (build_over_call): Build a vec of locations of the
> arguments before the call to maybe_constant_value, and pass to
> check_function_arguments.
> * cp-lang.c (LANG_HOOKS_GET_SUBSTRING_LOCATION): Define as
> c_get_substring_location.
> * parser.c (cp_parser_string_literal): Capture string
> concatenation locations.
>
> gcc/ChangeLog:
> PR c++/56856
> * input.c (expand_location_to_spelling_point): Add param "aspect"
> and use rather than hardcoding LOCATION_ASPECT_CARET.
> (get_substring_ranges_for_loc): Handle the case of a single token
> within a macro expansion.
> * input.h (expand_location_to_spelling_point): Add "aspect" param,
> defaulting to LOCATION_ASPECT_CARET.
>
> gcc/testsuite/ChangeLog:
> PR c++/56856
> * g++.dg/ext/builtin4.C: Set expected location for warning to the
> correct location within the format string.
> * g++.dg/plugin/plugin.exp (plugin_test_list): Add the plugin and
> files for testing locations within string literal locations from
> the C frontend.
> * g++.dg/warn/Wformat-method.C: New test.
> * g++.dg/warn/Wformat-pr71863.C: New test.
> * g++.dg/warn/Wformat-ranges-c++11.C: New test.
> * g++.dg/warn/Wformat-ranges.C: New test, based on
> gcc.dg/format/diagnostic-ranges.c.
> * gcc.dg/plugin/diagnostic-test-string-literals-1.c
> (test_multitoken_macro): Generalize expected output to work with
> both C and C++.
> * gcc.dg/plugin/diagnostic-test-string-literals-2.c
> (test_stringified_token_1): Likewise.
> (test_stringified_token_3): Likewise.
I typically leave the C++ bits to others, but this looks fine to me.
jeff
