On Thu, 2016-08-04 at 12:08 -0600, Jeff Law wrote: > On 08/03/2016 09:45 AM, David Malcolm wrote: > > This patch updates c-format.c to use the new class substring_loc, > > added > > in the previous patch, replacing location_column_from_byte_offset. > > Hence with this patch, Wformat can underline the precise erroneous > > format string in many more cases. > > > > The patch also introduces two new functions for emitting Wformat > > warnings: format_warning_at_substring and format_warning_at_char, > > providing an inform in the face of macros where the pertinent part > > of > > the format string may be separate from the function call. > > > > Successfully bootstrapped®rtested in conjunction with the rest > > of the > > patch kit on x86_64-pc-linux-gnu. > > > > (The v2 version of the patch had a successful selftest run for > > stage 1 on > > powerpc-ibm-aix7.1.3.0 (gcc111) in conjunction with the rest of the > > patch > > kit, and a successful build of stage1 for all targets via config > > -list.mk; > > the patch has only been rebased since) > > > > OK for trunk if it passes individual testing? (on top of patches 1 > > -2) > > > > gcc/c-family/ChangeLog: > > PR c/52952 > > * c-format.c: Include "diagnostic.h". > > (location_column_from_byte_offset): Delete. > > (location_from_offset): Delete. > > (format_warning_va): New function. > > (format_warning_at_substring): New function. > > (format_warning_at_char): New function. > > (check_format_arg): Capture location of format_tree and pass to > > check_format_info_main. > > (check_format_info_main): Add params FMT_PARAM_LOC and > > FORMAT_STRING_CST. Convert calls to warning_at to calls to > > format_warning_at_char. Pass a substring_loc instance to > > check_format_types. > > (check_format_types): Convert first param from a location_t > > to a const substring_loc & and rename to "fmt_loc". Attempt > > to extract the range of the relevant parameter and pass it > > to format_type_warning. > > (format_type_warning): Convert first param from a location_t > > to a const substring_loc & and rename to "fmt_loc". Add > > params "param_range" and "type". Replace calls to warning_at > > with calls to format_warning_at_substring. > > > > gcc/testsuite/ChangeLog: > > PR c/52952 > > * gcc.dg/cpp/pr66415-1.c: Likewise. > > * gcc.dg/format/asm_fprintf-1.c: Update column numbers. > > * gcc.dg/format/c90-printf-1.c: Likewise. > > * gcc.dg/format/diagnostic-ranges.c: New test case. > > --- > > > > > @@ -1758,6 +1859,7 @@ check_format_info_main (format_check_results > > *res, > > ++format_chars; > > continue; > > } > > + const char *start_of_this_format = format_chars; > Do you realize that this isn't used for ~700 lines after this point? > Is > there any sensible way to factor some code here to avoid the coding > disconnect. I realize the function was huge before you got in here, > but > if at all possible, I'd like to see a bit of cleanup. > > I think this is OK after that cleanup.
format_chars can get modified in numerous places in the intervening lines, which is why I stash the value there. I can do some kind of cleanup of check_format_info_main, maybe splitting out the things in the body of loop, moving them to support functions. That said, I note that Martin's sprintf patch: https://gcc.gnu.org/ml/gcc-patches/2016-07/msg00056.html also touches those ~700 lines in check_format_info_main in over a dozen places. Given that, would you prefer I do the cleanup before or after the substring_loc patch? [CCing Martin]
