On 10/26/17, Nathan Sidwell <nat...@acm.org> wrote: > On 10/26/2017 10:34 AM, David Malcolm wrote: >> [CCing Rainer and Mike for the gcc-dg.exp part] > >> Alternate idea: could show_column become a tri-state: >> * default: show non-zero columns >> * never: never show columns >> * always: always show a column, printing 0 for the no-column case >> and then use "always" in our testsuite > > One of the things this patch shows up is the number of places where > we're default accepting a zero column. IMHO it is best to explicitly > mark such tests. > >>> + size_t l = sprintf (result, col ? ":%d:%d" : ":%d", line, col); >> >> Possibly a silly question, but is it OK to have a formatted string >> call in which some of the arguments aren't consumed? (here "col" is only >> consumed for the true case, which consumes 2 arguments; it's not consumed >> for the false case). > > Yes.
I think I remember clang disagreeing; I remember it printing warnings from -Wformat-extra-args in a similar situation in gnulib's error_at_line module > >>> + gcc_checking_assert (l + 1 < sizeof (result)); >> >> Would snprintf be safer? > > I guess. but the assert's still needed. > >> Please create a selftest for the function, covering these cases: >> >> * line == 0 >> * line > 0 and col == 0 >> * line > 0 and col > 0 (checking output for these cases) >> * line == INT_MAX and col == INT_MAX (without checking output, just to >> tickle the assert) >> * line == INT_MIN and col == INT_MIN (likewise) > > Ok, I'll investigate this new fangled self-testing framework :) > >> There are some testcases where we deliberately don't have a *line* >> number; what happens to these? > > Those don't change. the dg-harness already does NOT expect a column > when lineno=0. > >> My Tcl skills aren't great, so hopefully someone else can review this; >> CCing Rainer and Mike. >> >> Also, is the proposed syntax for "no columns" OK? (note the tristate >> idea above) > > I'm not wedded to '-:', but as mentioned above, I think the tests should > be explicit about whether a column is expected or not (and the default > needs to be 'expect column', because of history) > > thanks for your comments. > > nathan > > -- > Nathan Sidwell >
