On Fri, 2018-02-09 at 13:01 +0000, Nick Clifton wrote:
> Hi David, Hi Martin,
>
> OK, take 3 of the patch is attached. In this version:
>
> * The escaping is handled by a new class.
> * Self-tests have been added (and they helped find a bug - yay!)
> * The documentation has been extended to mention -fmessage-length's
> effect on the #-directives, and to add documentation for #pragma
> GCC error and #pragma GCC warning. (Which appeared to be
> missing).
>
> I did try to add backslash characters to the regexp in the new
> testcase
> but I just could not find a way to persuade dejagnu to accept them.
>
> OK to apply ?
>
> (Apologies for the poor C++ coding - it is not my strong point. I
> am
> an assembler level programmer at heart).
>
> Cheers
> Nick
Thanks Nick, sorry for inflicting C++ on you.
This looks much better. Out of interest, what bug did the selftests
help find?
Various nitpicks below
> gcc/ChangeLog
> 2018-02-09 Nick Clifton <ni...@redhat.com>
>
> PR 84195
> * tree.c (escaped_string): New class. Converts an unescaped
> string into its escaped equivalent.
> (warn_deprecated_use): Use the new class to convert the
> deprecation message, if present.
> (test_escaped_strings): New self test.
> (test_c_tests): Add test_escaped_strings.
This doesn't mention the changes to the docs.
> gcc/testsuite/ChangeLog
> 2018-02-07 Nick Clifton <ni...@redhat.com>
>
> * gcc.c-torture/compile/pr84195.c: New test.
> Index: gcc/tree.c
> ===================================================================
> --- gcc/tree.c (revision 257519)
> +++ gcc/tree.c (working copy)
> @@ -12416,11 +12416,99 @@
> return is_typedef_decl (TYPE_NAME (type));
> }
>
> +// A class to handle converting a string that might contain
> +// control characters, (eg newline, form-feed, etc), into one
> +// in which contains escape sequences instead.
We're still mostly using C-style comments for blocks, though I don't
think we have an actual rule about this.
> +class escaped_string
> +{
> + public:
> + escaped_string () { m_owned = false; m_str = NULL; };
> + ~escaped_string () { if (m_owned) free (m_str); }
> + operator const char *() const { return (const char *) m_str; }
> + void escape (const char *);
> + private:
> + char * m_str;
> + bool m_owned;
> +};
I'd hoped that instead of this we could have an escape_string function
return an instance of a class that has responsibility for the "if
(ownership) free" dtor, but I don't think C++89 supports that (I had a
go at implementing it, but I think we'd need C++11's move semantics,
rather than just the NRVO).
So the approach above is OK by me (which, given that I suggested it,
may seem redundant :) )
> +void
> +escaped_string::escape (const char * unescaped)
> +{
> + /* PR 84195: Replace control characters in "unescaped" with their
> + escaped equivalents. Allow newlines if -fmessage-length has
> + been set to a non-zero value. This is done here, rather than
> + where the attribute is recorded as the message length can
> + change between these two locations. */
Move this comment to outside the function, as a descriptive comment for
the function as a whole.
> + char * escaped;
> + size_t i, new_i, len;
> +
> + if (m_owned)
> + free (m_str);
> +
> + m_str = (char *) unescaped;
> + m_owned = false;
> +
> + if (unescaped == NULL || *unescaped == 0)
> + return;
Is NULL a valid input here? If so, please add test coverage for that
to the selftest.
The "*unescaped == 0" is presumably a micro-optimization for the
strlen(escaped) == 0 case, as it appears the code already handles this
case.
> + len = strlen (unescaped);
> + escaped = NULL;
> + new_i = 0;
> +
> + for (i = 0; i < len; i++)
> + {
> + char c = unescaped[i];
> +
> + if (! ISCNTRL (c))
> + {
> + if (escaped)
> + escaped[new_i++] = c;
> + continue;
> + }
> +
> + if (c != '\n' || ! pp_is_wrapping_line (global_dc->printer))
> + {
> + if (escaped == NULL)
> + {
> + /* We only allocate space for a new string if we
> + actually encounter a control character that
> + needs replacing. */
> + escaped = (char *) xmalloc (len * 2 + 1);
> + strncpy (escaped, unescaped, i);
> + new_i = i;
> + }
> +
> + escaped [new_i++] = '\\';
Some of the spacing in this function looks a bit odd to my eyes, but
you're much more familiar with the GNU standards than me, and I don't
want to be too nitpicky (e.g. the space before an array access, and
the space after '!').
> +
> + switch (c)
> + {
> + case '\a': escaped[new_i++] = 'a'; break;
> + case '\b': escaped[new_i++] = 'b'; break;
> + case '\f': escaped[new_i++] = 'f'; break;
> + case '\n': escaped[new_i++] = 'n'; break;
> + case '\r': escaped[new_i++] = 'r'; break;
> + case '\t': escaped[new_i++] = 't'; break;
> + case '\v': escaped[new_i++] = 'v'; break;
> + default: escaped[new_i++] = '?'; break;
> + }
> + }
> + else if (escaped)
> + escaped[new_i++] = c;
> + }
> +
> + if (escaped)
> + {
> + escaped[new_i] = 0;
> + m_str = escaped;
> + m_owned = true;
> + }
> +}
> +
> /* Warn about a use of an identifier which was marked deprecated. */
> void
> warn_deprecated_use (tree node, tree attr)
> {
> - const char *msg;
> + class escaped_string msg;
"class" is redundant here.
> if (node == 0 || !warn_deprecated_decl)
> return;
> @@ -12442,9 +12530,7 @@
> attr = lookup_attribute ("deprecated", attr);
>
> if (attr)
> - msg = TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (attr)));
> - else
> - msg = NULL;
> + msg.escape (TREE_STRING_POINTER (TREE_VALUE (TREE_VALUE (attr))));
> bool w;
> if (DECL_P (node))
> @@ -12451,7 +12537,7 @@
> {
> if (msg)
> w = warning (OPT_Wdeprecated_declarations,
> - "%qD is deprecated: %s", node, msg);
> + "%qD is deprecated: %s", node, (const char *) msg);
> else
> w = warning (OPT_Wdeprecated_declarations,
> "%qD is deprecated", node);
> @@ -12478,7 +12564,7 @@
> {
> if (msg)
> w = warning (OPT_Wdeprecated_declarations,
> - "%qE is deprecated: %s", what, msg);
> + "%qE is deprecated: %s", what, (const char *) msg);
> else
> w = warning (OPT_Wdeprecated_declarations,
> "%qE is deprecated", what);
> @@ -12487,7 +12573,7 @@
> {
> if (msg)
> w = warning (OPT_Wdeprecated_declarations,
> - "type is deprecated: %s", msg);
> + "type is deprecated: %s", (const char *) msg);
> else
> w = warning (OPT_Wdeprecated_declarations,
> "type is deprecated");
> @@ -12501,7 +12587,7 @@
> {
> if (msg)
> warning (OPT_Wdeprecated_declarations, "%qE is deprecated: %s",
> - what, msg);
> + what, (const char *) msg);
> else
> warning (OPT_Wdeprecated_declarations, "%qE is deprecated",
> what);
> }
> @@ -12509,7 +12595,7 @@
> {
> if (msg)
> warning (OPT_Wdeprecated_declarations, "type is deprecated: %s",
> - msg);
> + (const char *) msg);
> else
> warning (OPT_Wdeprecated_declarations, "type is deprecated");
> }
> @@ -14548,6 +14634,43 @@
> check_strip_nops (wrapped_int_var, int_var);
> }
>
> +/* Check that string escaping works correctly. */
> +
> +static void
> +test_escaped_strings (void)
> +{
> + int saved_cutoff;
> + escaped_string msg;
> +
> + msg.escape ("");
> + ASSERT_STREQ ("", (const char *) msg);
> +
> + msg.escape ("foobar");
> + ASSERT_STREQ ("foobar", (const char *) msg);
> +
> + /* Ensure that we have -fmessage-length set to 0. */
> + saved_cutoff = pp_line_cutoff (global_dc->printer);
> + pp_line_cutoff (global_dc->printer) = 0;
> +
> + msg.escape ("foo\nbar");
> + ASSERT_STREQ ("foo\\nbar", (const char *) msg);
> +
> + msg.escape ("\a\b\f\n\r\t\v");
> + ASSERT_STREQ ("\\a\\b\\f\\n\\r\\t\\v", (const char *) msg);
> +
> + /* Now repeat the tests with -fmessage-length set to 5. */
> + pp_line_cutoff (global_dc->printer) = 5;
> +
> + msg.escape ("foo\nbar");
> + ASSERT_STREQ ("foo\nbar", (const char *) msg);
> +
> + msg.escape ("\a\b\f\n\r\t\v");
> + ASSERT_STREQ ("\\a\\b\\f\n\\r\\t\\v", (const char *) msg);
> +
> + /* Restore the original message length setting. */
> + pp_line_cutoff (global_dc->printer) = saved_cutoff;
> +}
> +
> /* Run all of the selftests within this file. */
>
> void
> @@ -14558,6 +14681,7 @@
> test_labels ();
> test_vector_cst_patterns ();
> test_location_wrappers ();
> + test_escaped_strings ();
> }
>
> } // namespace selftest
> Index: gcc/doc/extend.texi
> ===================================================================
> --- gcc/doc/extend.texi (revision 257519)
> +++ gcc/doc/extend.texi (working copy)
> @@ -2563,6 +2563,9 @@
> The @code{deprecated} attribute can also be used for variables and
> types (@pxref{Variable Attributes}, @pxref{Type Attributes}.)
>
> +The message attached to the attribute is affected by the setting of
> +the @option{-fmessage-length} option.
> +
> @item error ("@var{message}")
> @itemx warning ("@var{message}")
> @cindex @code{error} function attribute
> @@ -6077,6 +6080,9 @@
> types (@pxref{Common Function Attributes},
> @pxref{Common Type Attributes}).
>
> +The message attached to the attribute is affected by the setting of
> +the @option{-fmessage-length} option.
> +
> @item nonstring
> @cindex @code{nonstring} variable attribute
> The @code{nonstring} variable attribute specifies that an object or member
> @@ -7035,11 +7041,16 @@
> deprecated. Line 5 has no warning because T3 is explicitly
> deprecated. Similarly for line 6. The optional @var{msg}
> argument, which must be a string, is printed in the warning if
> -present.
> +present. Control characters in the string will be replaced with
> +spaces, and if the @option{-fmessage-length} option is set to 0 (its
> +default value) then any newline characters will also be replaced.
>
> The @code{deprecated} attribute can also be used for functions and
> variables (@pxref{Function Attributes}, @pxref{Variable Attributes}.)
>
> +The message attached to the attribute is affected by the setting of
> +the @option{-fmessage-length} option.
> +
> @item designated_init
> @cindex @code{designated_init} type attribute
> This attribute may only be applied to structure types. It indicates
> @@ -22372,7 +22383,9 @@
> @cindex pragma, diagnostic
>
> Prints @var{string} as a compiler message on compilation. The message
> -is informational only, and is neither a compilation warning nor an error.
> +is informational only, and is neither a compilation warning nor an
> +error. Newlines can be included in the string by using the @samp{\n}
> +escape sequence.
>
> @smallexample
> #pragma message "Compiling " __FILE__ "..."
> @@ -22392,6 +22405,37 @@
> prints @samp{/tmp/file.c:4: note: #pragma message:
> TODO - Remember to fix this}.
>
> +@item #pragma GCC error @var{message}
> +@cindex pragma, diagnostic
> +Generates an error message. This pragma @emph{is} considered to
> +indicate an error in the compilation, and it will be treated as such.
> +
> +Newlines can be included in the string by using the @samp{\n}
> +escape sequence. They will be displayed as newlines even if the
> +@option{-fmessage-length} option is set to zero.
> +
> +The error is only generated if the pragma is present in the code after
> +pre-processing has been completed. It does not matter however if the
> +code containing the pragma is unreachable:
> +
> +@smallexample
> +#if 0
> +#pragma GCC error "this error is not seen"
> +#endif
> +void foo (void)
> +@{
> + return;
> +#pragma GCC error "this error is seen"
> +@}
> +@end smallexample
> +
> +@item #pragma GCC warning @var{message}
> +@cindex pragma, diagnostic
> +This is just like @samp{pragma GCC error} except that a warning
> +message is issued instead of an error message. Unless
> +@option{-Werror} is in effect, in which case this pragma will generate
> +an error as well.
> +
> @end table
>
> @node Visibility Pragmas
> Index: gcc/doc/invoke.texi
> ===================================================================
> --- gcc/doc/invoke.texi (revision 257519)
> +++ gcc/doc/invoke.texi (working copy)
> @@ -3496,6 +3496,11 @@
> done; each error message appears on a single line. This is the
> default for all front ends.
>
> +Note - this option also affects the display of the @samp{#error} and
> +@samp{#warning} pre-processor directives, and the @samp{deprecated}
> +function/type/variable attribute. It does not however affect the
> +@samp{pragma GCC warning} and @samp{pragma GCC error} pragmas.
> +
> @item -fdiagnostics-show-location=once
> @opindex fdiagnostics-show-location
> Only meaningful in line-wrapping mode. Instructs the diagnostic messages
> --- /dev/null 2018-02-08 08:21:34.475648704 +0000
> +++ gcc/testsuite/gcc.c-torture/compile/pr84195.c 2018-02-07
> 17:20:13.494587052 +0000
> @@ -0,0 +1,17 @@
> +/* { dg-options "-Wdeprecated-declarations" } */
> +
> +/* Check that MSG is printed without the escape characters being interpreted.
> + Especially the newlines.
> +
> + Note - gcc's behaviour is inconsistent in this regard as #error and
> + #warning will also display control characters as escape sequences,
> + whereas #pragma GCC error and #pragma GCC warning will perform the
> + control operations of the control characters. */
> +
> +#define MSG "foo\n\t\rbar"
> +
> +int f (int i __attribute__ ((deprecated (MSG))))
> +{
> + return 0 ? i : 0; /* { dg-warning "'i' is deprecated: foo.n.t.rbar" } */
> +}
Thanks for updating/fixing the docs.
Other than the nitpicks above, this looks good.
I believe I can approve this with my "diagnostics messages" maintainer
hat on, but given that we're deep in stage 4 and this isn't a
regression, please queue it up for gcc 9 stage 1 (unless there's a
pressing need to get this into gcc 8, in which case talk to the release
managers!)
Dave
