On 08/10/2016 06:04 AM, Richard Biener wrote:
On Tue, Aug 9, 2016 at 3:17 PM, Aldy Hernandez <al...@redhat.com> wrote:
On 08/05/2016 01:55 PM, Richard Biener wrote:
Hi Richard.
Please don't use std::string. For string building you can use obstacks.
Alright let's talk details then so I can write things up in a way you
approve of.
Take for instance simple uses like all the tree_*check_failed routines,
which I thought were great candidates for std::string-- they're going to be
outputted to the screen or disk which is clearly many times more expensive
than the malloc or overhead of std::string:
length += strlen ("expected ");
buffer = tmp = (char *) alloca (length);
length = 0;
while ((code = (enum tree_code) va_arg (args, int)))
{
const char *prefix = length ? " or " : "expected ";
strcpy (tmp + length, prefix);
length += strlen (prefix);
strcpy (tmp + length, get_tree_code_name (code));
length += strlen (get_tree_code_name (code));
}
Do you suggest using obstacks here, or did you have something else in mind?
Why would you want to get rid of the alloca here?
How about if it's something like the above but there are multiple exit
points from the function. I mean, we still need to call obstack_free() on
all exit points, which is what I wanted to avoid for something as simple as
building a throw-away string.
But you'll end up in
internal_error ("tree check: %s, have %s in %s, at %s:%d",
buffer, get_tree_code_name (TREE_CODE (node)),
function, trim_filename (file), line);
where you have an interface using C strings again.
It's not that the standard library is bad per-se, it's just using a
tool that doesn't
fit its uses. This makes the code a messy mix of two concepts which is what
I object to.
Yes, the above example may have premature optimization applied. Is that
a reason to re-write it using std::string? No. Is it a reason to rewrite it
using obstracks? No. Just leave it alone.
How about we use auto_vec<> with an expected sane default? This way we
have a performance conscious solution that will fallback to malloc if
necessary, while cleaning up after itself without the need for changing
every exit point from a function.
For example, the attached untested patch implements this approach for
tree.c.
Would this be acceptable?
Aldy
diff --git a/gcc/tree.c b/gcc/tree.c
index 11d3b51..160c539 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -9636,9 +9636,9 @@ anon_aggrname_format()
tree
get_file_function_name (const char *type)
{
- char *buf;
const char *p;
char *q;
+ auto_vec<char, 100> chunk;
/* If we already have a name we know to be unique, just use that. */
if (first_global_object_name)
@@ -9674,7 +9674,8 @@ get_file_function_name (const char *type)
file = LOCATION_FILE (input_location);
len = strlen (file);
- q = (char *) alloca (9 + 17 + len + 1);
+ chunk.reserve (9 + 17 + len + 1);
+ q = chunk.address ();
memcpy (q, file, len + 1);
snprintf (q + len, 9 + 17 + 1, "_%08X_" HOST_WIDE_INT_PRINT_HEX,
@@ -9684,16 +9685,16 @@ get_file_function_name (const char *type)
}
clean_symbol_name (q);
- buf = (char *) alloca (sizeof (FILE_FUNCTION_FORMAT) + strlen (p)
- + strlen (type));
/* Set up the name of the file-level functions we may need.
Use a global object (which is already required to be unique over
the program) rather than the file name (which imposes extra
constraints). */
- sprintf (buf, FILE_FUNCTION_FORMAT, type, p);
+ auto_vec<char, 100> buf;
+ buf.reserve (sizeof (FILE_FUNCTION_FORMAT) + strlen (p) + strlen (type));
+ sprintf (buf.address (), FILE_FUNCTION_FORMAT, type, p);
- return get_identifier (buf);
+ return get_identifier (buf.address ());
}
#if defined ENABLE_TREE_CHECKING && (GCC_VERSION >= 2007)
@@ -9711,6 +9712,7 @@ tree_check_failed (const_tree node, const char *file,
const char *buffer;
unsigned length = 0;
enum tree_code code;
+ auto_vec<char, 150> chunk;
va_start (args, function);
while ((code = (enum tree_code) va_arg (args, int)))
@@ -9721,7 +9723,8 @@ tree_check_failed (const_tree node, const char *file,
char *tmp;
va_start (args, function);
length += strlen ("expected ");
- buffer = tmp = (char *) alloca (length);
+ chunk.reserve (length);
+ buffer = tmp = chunk.address ();
length = 0;
while ((code = (enum tree_code) va_arg (args, int)))
{
@@ -9760,7 +9763,9 @@ tree_not_check_failed (const_tree node, const char *file,
length += 4 + strlen (get_tree_code_name (code));
va_end (args);
va_start (args, function);
- buffer = (char *) alloca (length);
+ auto_vec<char, 150> chunk;
+ chunk.reserve (length);
+ buffer = chunk.address ();
length = 0;
while ((code = (enum tree_code) va_arg (args, int)))
{
@@ -9809,7 +9814,9 @@ tree_range_check_failed (const_tree node, const char
*file, int line,
length += 4 + strlen (get_tree_code_name ((enum tree_code) c));
length += strlen ("expected ");
- buffer = (char *) alloca (length);
+ auto_vec<char, 150> chunk;
+ chunk.reserve (length);
+ buffer = chunk.address ();
length = 0;
for (c = c1; c <= c2; ++c)
@@ -9870,7 +9877,9 @@ omp_clause_range_check_failed (const_tree node, const
char *file, int line,
length += 4 + strlen (omp_clause_code_name[c]);
length += strlen ("expected ");
- buffer = (char *) alloca (length);
+ auto_vec<char, 150> chunk;
+ chunk.reserve (length);
+ buffer = chunk.address ();
length = 0;
for (c = c1; c <= c2; ++c)
