| Message ID | 79de46b9-9770-02dd-6d98-9c72b7e6c59d@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 26 Oct 2016, Martin Sebor wrote: > The attached patch implements one such approach by having the pretty > printer recognize the space format flag to suppress the type suffix, > so "%E" still prints the suffix but "% E" does not. I did this to > preserve the existing output but I think it would be nicer to avoid > printing the suffix with %E and treat (for instance) the pound sign > as a request to add the suffix. I have tested the attached patch > but not the alternative. I think printing the suffixes is a relic of %E being used to print full expressions. It's established by now that printing expressions reconstructed from trees is a bad idea; we can get better results by having precise location ranges and underlining the relevant part of the source. So if we could make sure nowhere is trying the use %E (or %qE, etc.) with expressions that might not be constants, where the type might be relevant, then we'd have confidence that stopping printing the suffix is safe. But given the low quality of the reconstructed expressions, it's probably safe anyway. (Most %qE uses are for identifiers not expressions. If we give identifiers a different static type from "tree" - and certainly there isn't much reason for them to have the same type as expressions - then we'll need to change the format for either identifiers or expressions.) -- Joseph S. Myers joseph@codesourcery.com
On 10/26/2016 10:37 AM, Martin Sebor wrote: > When formatting an integer constant using the %E directive GCC > includes a suffix that indicates its type. This can perhaps be > useful in some situations but in my experience it's distracting > and gets in the way when writing tests. > > Here's an example: > > $ cat b.c && gcc b.c > constexpr __SIZE_TYPE__ x = 2; > > enum E: bool { e = x }; > b.c:3:20: error: enumerator value 2ul is outside the range of > underlying type ‘bool’ > enum E: bool { e = x }; > ^ > > Notice the "2ul" in the error message. > > As far as I can tell, Clang avoids printing the suffix and I think > it would be nice if the GCC pretty printer made it possible to avoid > it as well. > > The attached patch implements one such approach by having the pretty > printer recognize the space format flag to suppress the type suffix, > so "%E" still prints the suffix but "% E" does not. I did this to > preserve the existing output but I think it would be nicer to avoid > printing the suffix with %E and treat (for instance) the pound sign > as a request to add the suffix. I have tested the attached patch > but not the alternative. > > Does anyone have any comments/suggestions for which of the two > approaches would be preferable (or what I may have missed here)? > I CC David as the diagnostic maintainer. I'm having a hard time seeing how this is a significant issue, even when writing tests. It also seems to me that relaying the type of the constant as a suffix would help in cases that aren't so obvious. What am I missing? Jeff
On 11/16/2016 11:34 AM, Jeff Law wrote: > On 10/26/2016 10:37 AM, Martin Sebor wrote: >> When formatting an integer constant using the %E directive GCC >> includes a suffix that indicates its type. This can perhaps be >> useful in some situations but in my experience it's distracting >> and gets in the way when writing tests. >> >> Here's an example: >> >> $ cat b.c && gcc b.c >> constexpr __SIZE_TYPE__ x = 2; >> >> enum E: bool { e = x }; >> b.c:3:20: error: enumerator value 2ul is outside the range of >> underlying type ‘bool’ >> enum E: bool { e = x }; >> ^ >> >> Notice the "2ul" in the error message. >> >> As far as I can tell, Clang avoids printing the suffix and I think >> it would be nice if the GCC pretty printer made it possible to avoid >> it as well. >> >> The attached patch implements one such approach by having the pretty >> printer recognize the space format flag to suppress the type suffix, >> so "%E" still prints the suffix but "% E" does not. I did this to >> preserve the existing output but I think it would be nicer to avoid >> printing the suffix with %E and treat (for instance) the pound sign >> as a request to add the suffix. I have tested the attached patch >> but not the alternative. >> >> Does anyone have any comments/suggestions for which of the two >> approaches would be preferable (or what I may have missed here)? >> I CC David as the diagnostic maintainer. > I'm having a hard time seeing how this is a significant issue, even when > writing tests. > > It also seems to me that relaying the type of the constant as a suffix > would help in cases that aren't so obvious. > > What am I missing? I don't think it's terribly important, more like nuisance. Tests that check the value printed by the %E directive (I've been writing lots of those lately -- see for example (*)) have to consistently use this pattern: \[0-9\]+\[lu\]* When the type of the %E argument is a type like size_t or similar that can be an alias for unsigned long or unsigned int, it's easy to make a mistake and hardcode either \[0-9\]+lu or \[0-9\]+u based on the target where the test is being developed and end up with failures on targets where the actual type is the other. Copying test cases exercising one type to those exercising the other (say from int to long) is also more tedious than it would be without the suffix. Beyond tests, I have never found the suffix helpful in warnings or errors, but I also haven't seen too many of them in released versions of GCC. With the work I've been doing on buffer overflow where size expressions are routinely included in the diagnostics, there are lots more of them. In some (e.g., in all the -Wformat-length) I've taken care to avoid printing the suffix by converting tree nodes to HOST_WIDE_INT. But that's cumbersome and error-prone, and leads to inconsistent output from GCC for different diagnostics that don't do the same. Martin [*] https://gcc.gnu.org/ml/gcc-patches/2016-11/msg01672.html
gcc/c/ChangeLog: 2016-10-26 Martin Sebor <msebor@redhat.com> * c-objc-common.c (c_tree_printer::get_flag): Declare new function (c_tree_printer::set_flag): Same. gcc/c-family/ChangeLog: 2016-10-26 Martin Sebor <msebor@redhat.com> * c-pretty-print.c (c_tree_printer::get_flag): Define. (c_tree_printer::set_flag): Define. (pp_c_integer_constant): Don't print type suffix when space is set in flags. * c-pretty-print.h (pp_c_flag_space): Add enumerator. gcc/cp/ChangeLog: 2016-10-26 Martin Sebor <msebor@redhat.com> * error.c (cp_printer): Set space in flags. gcc/ChangeLog: 2016-10-26 Martin Sebor <msebor@redhat.com> * pretty-print.c (pp_format): Recognize space. * pretty-print.h (pretty_printer::get_flag): New function. (pretty_printer::set_flag): Same. diff --git a/gcc/c-family/c-pretty-print.c b/gcc/c-family/c-pretty-print.c index 90428ca..7d9375d 100644 --- a/gcc/c-family/c-pretty-print.c +++ b/gcc/c-family/c-pretty-print.c @@ -292,6 +292,26 @@ pp_c_pointer (c_pretty_printer *pp, tree t) } } +bool +c_pretty_printer::get_flag (char flag) +{ + return ' ' == flag && (flags & pp_c_flag_space); +} + +bool +c_pretty_printer::set_flag (char flag, bool value) +{ + gcc_assert (flag == ' '); + + bool previous = c_pretty_printer::get_flag (flag); + if (value) + flag |= pp_c_flag_space; + else + flag &= ~pp_c_flag_space; + + return previous; +} + /* simple-type-specifier: type-specifier @@ -926,24 +946,34 @@ pp_c_integer_constant (c_pretty_printer *pp, tree i) print_hex (wi, pp_buffer (pp)->digit_buffer); pp_string (pp, pp_buffer (pp)->digit_buffer); } - if (TYPE_UNSIGNED (type)) - pp_character (pp, 'u'); - if (type == long_integer_type_node || type == long_unsigned_type_node) - pp_character (pp, 'l'); - else if (type == long_long_integer_type_node - || type == long_long_unsigned_type_node) - pp_string (pp, "ll"); - else for (idx = 0; idx < NUM_INT_N_ENTS; idx ++) - if (int_n_enabled_p[idx]) - { - char buf[2+20]; - if (type == int_n_trees[idx].signed_type - || type == int_n_trees[idx].unsigned_type) - { - sprintf (buf, "I%d", int_n_data[idx].bitsize); - pp_string (pp, buf); - } - } + + if (pp->get_flag (' ')) + { + if (TYPE_UNSIGNED (type)) + pp_character (pp, 'u'); + + if (type == long_integer_type_node || type == long_unsigned_type_node) + pp_character (pp, 'l'); + else if (type == long_long_integer_type_node + || type == long_long_unsigned_type_node) + pp_string (pp, "ll"); + else + { + for (idx = 0; idx < NUM_INT_N_ENTS; idx ++) + { + if (int_n_enabled_p[idx]) + { + char buf[2+20]; + if (type == int_n_trees[idx].signed_type + || type == int_n_trees[idx].unsigned_type) + { + sprintf (buf, "I%d", int_n_data[idx].bitsize); + pp_string (pp, buf); + } + } + } + } + } } /* Print out a CHARACTER literal. */ diff --git a/gcc/c-family/c-pretty-print.h b/gcc/c-family/c-pretty-print.h index 253f192..10ad5d9 100644 --- a/gcc/c-family/c-pretty-print.h +++ b/gcc/c-family/c-pretty-print.h @@ -30,7 +30,8 @@ enum pp_c_pretty_print_flags { pp_c_flag_abstract = 1 << 1, pp_c_flag_gnu_v3 = 1 << 2, - pp_c_flag_last_bit = 3 + pp_c_flag_space = 1 << 3, + pp_c_flag_last_bit = 4 }; @@ -51,6 +52,10 @@ struct c_pretty_printer : pretty_printer { c_pretty_printer (); + /* Get and set the format flag. */ + virtual bool get_flag (char); + virtual bool set_flag (char, bool); + // Format string, possibly translated. void translate_string (const char *); diff --git a/gcc/c/c-objc-common.c b/gcc/c/c-objc-common.c index 20dc024..1bd60cf 100644 --- a/gcc/c/c-objc-common.c +++ b/gcc/c/c-objc-common.c @@ -99,6 +99,12 @@ c_tree_printer (pretty_printer *pp, text_info *text, const char *spec, text->set_location (0, DECL_SOURCE_LOCATION (t), true); } + if (' ' == *spec) + { + pp->set_flag (' ', true); + ++spec; + } + switch (*spec) { case 'D': diff --git a/gcc/cp/error.c b/gcc/cp/error.c index 20b20b4..d24ae26 100644 --- a/gcc/cp/error.c +++ b/gcc/cp/error.c @@ -3539,6 +3539,12 @@ cp_printer (pretty_printer *pp, text_info *text, const char *spec, if (precision != 0 || wide) return false; + if (*spec == ' ') + { + pp->set_flag (' '); + ++spec; + } + switch (*spec) { case 'A': result = args_to_string (next_tree, verbose); break; diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c index a39815e..93b7419 100644 --- a/gcc/pretty-print.c +++ b/gcc/pretty-print.c @@ -293,6 +293,11 @@ pp_indent (pretty_printer *pp) %.*s: a substring the length of which is specified by an argument integer. %Ns: likewise, but length specified as constant in the format string. + + Flag 'w': wide integer (HOST_WIDE_INT). + Flag '+': always print a sign for integer and real constants. + Flag ' ': printer-specific. + Flag '#': printer-specific. Flag 'q': quote formatted text (must come immediately after '%'). Arguments can be used sequentially, or through %N$ resp. *N$ @@ -425,12 +430,14 @@ pp_format (pretty_printer *pp, text_info *text) gcc_assert (argno < PP_NL_ARGMAX); gcc_assert (!formatters[argno]); formatters[argno] = &args[chunk]; + + /* Add known format flags. */ do { obstack_1grow (&buffer->chunk_obstack, *p); p++; } - while (strchr ("qwl+#", p[-1])); + while (strchr ("qwl+# ", p[-1])); if (p[-1] == '.') { diff --git a/gcc/pretty-print.h b/gcc/pretty-print.h index f49f35a..9d4386f 100644 --- a/gcc/pretty-print.h +++ b/gcc/pretty-print.h @@ -213,6 +213,13 @@ struct pretty_printer virtual ~pretty_printer (); + /* Get and set the format flag. */ + virtual bool get_flag (char) { return false; } + virtual bool set_flag (char, bool) { return false; } + + /* Not virtual. Provided for convenience. */ + bool set_flag (char flag) { return set_flag (flag, true); } + /* Where we print external representation of ENTITY. */ output_buffer *buffer;
When formatting an integer constant using the %E directive GCC includes a suffix that indicates its type. This can perhaps be useful in some situations but in my experience it's distracting and gets in the way when writing tests. Here's an example: $ cat b.c && gcc b.c constexpr __SIZE_TYPE__ x = 2; enum E: bool { e = x }; b.c:3:20: error: enumerator value 2ul is outside the range of underlying type ‘bool’ enum E: bool { e = x }; ^ Notice the "2ul" in the error message. As far as I can tell, Clang avoids printing the suffix and I think it would be nice if the GCC pretty printer made it possible to avoid it as well. The attached patch implements one such approach by having the pretty printer recognize the space format flag to suppress the type suffix, so "%E" still prints the suffix but "% E" does not. I did this to preserve the existing output but I think it would be nicer to avoid printing the suffix with %E and treat (for instance) the pound sign as a request to add the suffix. I have tested the attached patch but not the alternative. Does anyone have any comments/suggestions for which of the two approaches would be preferable (or what I may have missed here)? I CC David as the diagnostic maintainer. Thanks Martin