On 02/16/2018 05:43 AM, Nick Clifton wrote:
> Hi David,
>
> Attached is a revised version of the patch which I hope addresses all
> of your (very helpful) comments on the v3 patch.
>
> OK to apply once the sources are back on stage 1 ?
>
> Cheers
> Nick
>
> 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.
> * doc/extend.texi (deprecated): Add a note that the
> deprecation message is affected by the -fmessage-length
> option, and that control characters will be escaped.
> (#pragma GCC error): Document this pragma.
> (#pragma GCC warning): Likewise.
> * doc/invoke.texi (-fmessage-length): Document this option's
> effect on the #warning and #error preprocessor directives and
> the deprecated attribute.
>
> gcc/testsuite/ChangeLog
> 2018-02-09 Nick Clifton <ni...@redhat.com>
>
> PR 84195
> * gcc.c-torture/compile/pr84195.c: New test.
The docs still say "Control characters in the string will be replaced
with spaces", but they're being escaped now. That needs to be fixed.
I note you overload the cast operator in your new class. Why not just
use an accessor? Was this style something someone else suggested?
Onward to the nits :-)
+ char *m_str;
+ bool m_owned;
I don't think we try to line up variable definitions like that.
+ escaped = (char *) xmalloc (len * 2 + 1);
I believe that generally we want a slower growth pattern. You'll find
that we regularly do something like len * 3 / 2 + 1. Seems wise here too.
+void
+escaped_string::escape (const char * unescaped)
No need for the space between the * and unescaped.
+ 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;
+ }
I believe our coding standards would have the statements on new lines
rather than on the line with the case label.
So I think the biggest question here is the cast overload vs just using
an accessor.
Jeff
