Re: [GSoC] use obstack in parse_c_expr

[Richard Biener <richard dot guenther at gmail dot com>]

On Fri, Apr 25, 2014 at 2:01 PM, Trevor Saunders <tsaunders@mozilla.com
wrote:
> On Fri, Apr 25, 2014 at 05:09:57PM +0530, Prathamesh Kulkarni wrote:
>> On Thu, Apr 24, 2014 at 9:18 PM, Diego Novillo <dnovillo@google.com> wrote:
>> > Please remember to send patches to gcc-patches.
>> >
>> > I'm wondering if you couldn't just use std::string here.  No need to change
>> > one complicated allocation scheme with another.
>
> I was thinking about suggesting that too, a real string APi (and
> std::string is the easiest to hand) seems much less error prone.
>
>> Since we were using c-styled strings throughout genmatch, I thought of
>> using obstack instead of std::string.
>> This patch uses std::string in parse_c_expr to build c-code string.
>> Should we also change AST (operand and subclasses) to use std::string
>> instead of c-styled strings ?
>
> it might make things nicer, and it would save you the strdup at the end
> of this function, not that performance matters here.  So I'd approve of
> doing that.

Indeed.  It's only a generator program, so using std::string is fine.

Thanks,
Richard.

> Trev
>
>>
>> Thanks and Regards,
>> Prathamesh
>> >
>> >
>> > Diego.
>> >
>> >
>> > On Thu, Apr 24, 2014 at 8:15 AM, Prathamesh Kulkarni
>> > <bilbotheelffriend@gmail.com> wrote:
>> >>
>> >> Hi,
>> >>    There was a comment in parse_c_expr, mentioning to use obstack to
>> >> build c-code string. I have attached patch for the same.
>> >> OK to commit ?
>> >>
>> >> * genmatch.c (parse_c_expr): Use obstack to build c code string.
>> >>
>> >> Thanks and Regards,
>> >> Prathamesh
>> >
>> >
>
>> Index: gcc/genmatch.c
>> ===================================================================
>> --- gcc/genmatch.c    (revision 209470)
>> +++ gcc/genmatch.c    (working copy)
>> @@ -29,7 +29,7 @@ along with GCC; see the file COPYING3.
>>  #include "hashtab.h"
>>  #include "hash-table.h"
>>  #include "vec.h"
>> -
>> +#include <string>
>>
>>  /* Grammar
>>
>> @@ -689,15 +689,17 @@ parse_c_expr (cpp_reader *r, cpp_ttype s
>>    cpp_ttype end;
>>    unsigned opencnt;
>>    char *code;
>> +  std::string cstr;
>> +
>>    eat_token (r, start);
>>    if (start == CPP_OPEN_PAREN)
>>      {
>> -      code = xstrdup ("(");
>> +      cstr += "(";
>>        end = CPP_CLOSE_PAREN;
>>      }
>>    else if (start == CPP_OPEN_BRACE)
>>      {
>> -      code = xstrdup ("({");
>> +      cstr += "({";
>>        end = CPP_CLOSE_BRACE;
>>      }
>>    else
>> @@ -714,13 +716,9 @@ parse_c_expr (cpp_reader *r, cpp_ttype s
>>         if (n->type == CPP_NUMBER
>>             && !(n->flags & PREV_WHITE))
>>           {
>> -           code = (char *)xrealloc (code, strlen (code)
>> -                                    + strlen ("captures[") + 4);
>>             if (token->flags & PREV_WHITE)
>> -             strcat (code, " ");
>> -           strcat (code, "captures[");
>> -           strcat (code, get_number (r));
>> -           strcat (code, "]");
>> +             cstr += " ";
>> +           cstr.append ("captures[").append (get_number (r)).append ("]");
>>             continue;
>>           }
>>       }
>> @@ -734,24 +732,19 @@ parse_c_expr (cpp_reader *r, cpp_ttype s
>>
>>        /* Output the token as string.  */
>>        char *tk = (char *)cpp_token_as_text (r, token);
>> -      code = (char *)xrealloc (code, strlen (code) + strlen (tk) + 2);
>>        if (token->flags & PREV_WHITE)
>> -     strcat (code, " ");
>> -      strcat (code, tk);
>> +     cstr += " ";
>> +      cstr += tk;
>>      }
>>    while (1);
>>    if (end == CPP_CLOSE_PAREN)
>> -    {
>> -      code = (char *)xrealloc (code, strlen (code) + 1 + 1);
>> -      strcat (code, ")");
>> -    }
>> +    cstr += ")";
>>    else if (end == CPP_CLOSE_BRACE)
>> -    {
>> -      code = (char *)xrealloc (code, strlen (code) + 1 + 2);
>> -      strcat (code, "})");
>> -    }
>> +    cstr += "})";
>>    else
>>      gcc_unreachable ();
>> +
>> +  code = (char *) xstrdup (cstr.c_str ());
>>    return new c_expr (code);
>>  }
>>
>