Re: [GSoC] use obstack in parse_c_expr

Re: [GSoC] use obstack in parse_c_expr

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

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.
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 ?

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
>
>

[-- Attachment #2: string.patch --]
[-- Type: text/x-patch, Size: 2068 bytes --]

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);
 }
 

Re: [GSoC] use obstack in parse_c_expr

[Trevor Saunders]

[-- Attachment #1: Type: text/plain, Size: 3736 bytes --]

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.

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);
>  }
>  


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [GSoC] use obstack in parse_c_expr

[Richard Biener]

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);
>>  }
>>
>

Re: [GSoC] use obstack in parse_c_expr

[Jeff Law]

On 04/25/14 05:39, 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.
> 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 ?
Yes, let's go with this.

Jeff

Re: [GSoC] use obstack in parse_c_expr

[Richard Biener]

On Fri, May 9, 2014 at 10:30 PM, Jeff Law <law@redhat.com> wrote:
> On 04/25/14 05:39, 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.
>>
>> 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 ?
>
> Yes, let's go with this.

Btw, how is the status with the copyright assignments?  Can we give
the GSOC students svn write access so they can commit to the
branches?  Or should we direct them to use git branches off the
github.com git mirror?

Thanks,
Richard.

> Jeff