Re: [RFA] C++-ify parser_state

[Simon Marchi <simon.marchi@ericsson.com>]

Hi Tom,

I think this looks good.  I have some suggestions to clean things a further
bit, you are free to take them or leave them

On 2017-11-26 12:40 PM, Tom Tromey wrote:

> +expression_up
> +parser_state::release ()
>  {
>    /* Record the actual number of expression elements, and then
>       reallocate the expression memory so that we free up any
>       excess elements.  */
>  
> -  ps->expout->nelts = ps->expout_ptr;
> -  ps->expout = (struct expression *)
> -     xrealloc (ps->expout,
> +  expout->nelts = expout_ptr;
> +  expout = (struct expression *)
> +     xrealloc (expout,
>  	       sizeof (struct expression)
> -	       + EXP_ELEM_TO_BYTES (ps->expout_ptr));
> +	       + EXP_ELEM_TO_BYTES (expout_ptr));
> +
> +  expression_up result (expout);
> +  /* Ensure that we don't free it in the destructor.  */
> +  expout = nullptr;
> +  return result;

If expout was an expression_up, we could just std::move it here, and
wouldn't need an explicit destructor.

>  }
>  
>  /* This page contains the functions for adding data to the struct expression
> @@ -1118,7 +1127,6 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc,
>  			int comma, int void_context_p, int *out_subexp)
>  {
>    const struct language_defn *lang = NULL;
> -  struct parser_state ps;
>    int subexp;
>  
>    lexptr = *stringptr;
> @@ -1194,7 +1202,7 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc,
>       and others called from *.y) ensure CURRENT_LANGUAGE gets restored
>       to the value matching SELECTED_FRAME as set by get_current_arch.  */
>  
> -  initialize_expout (&ps, 10, lang, get_current_arch ());
> +  parser_state ps (10, lang, get_current_arch ());
>  
>    scoped_restore_current_language lang_saver;
>    set_language (lang->la_language);
> @@ -1207,33 +1215,32 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc,
>    CATCH (except, RETURN_MASK_ALL)
>      {
>        if (! parse_completion)
> -	{
> -	  xfree (ps.expout);
> -	  throw_exception (except);
> -	}
> +	throw_exception (except);
>      }
>    END_CATCH
>  
> -  reallocate_expout (&ps);
> +  /* We have to operate on an "expression *", due to la_post_parser,
> +     which explains this funny-looking double release.  */
> +  struct expression *result = ps.release ().release ();
>  
>    /* Convert expression from postfix form as generated by yacc
>       parser, to a prefix form.  */
>  
>    if (expressiondebug)
> -    dump_raw_expression (ps.expout, gdb_stdlog,
> +    dump_raw_expression (result, gdb_stdlog,
>  			 "before conversion to prefix form");
>  
> -  subexp = prefixify_expression (ps.expout);
> +  subexp = prefixify_expression (result);
>    if (out_subexp)
>      *out_subexp = subexp;
>  
> -  lang->la_post_parser (&ps.expout, void_context_p);
> +  lang->la_post_parser (&result, void_context_p);

Passing a pointer or reference to the unique_ptr would allow
the implementations of la_post_parser to modify it directly,
and avoid the .release ().release ().

> --- a/gdb/stap-probe.c
> +++ b/gdb/stap-probe.c
> @@ -1146,25 +1146,17 @@ static expression_up
>  stap_parse_argument (const char **arg, struct type *atype,
>  		     struct gdbarch *gdbarch)
>  {
> -  struct stap_parse_info p;
> -  struct cleanup *back_to;
> -
>    /* We need to initialize the expression buffer, in order to begin
>       our parsing efforts.  We use language_c here because we may need
>       to do pointer arithmetics.  */
> -  initialize_expout (&p.pstate, 10, language_def (language_c), gdbarch);
> -  back_to = make_cleanup (free_current_contents, &p.pstate.expout);
> +  struct stap_parse_info p (10, language_def (language_c), gdbarch);
>  
>    p.saved_arg = *arg;
>    p.arg = *arg;
>    p.arg_type = atype;
> -  p.gdbarch = gdbarch;
> -  p.inside_paren_p = 0;

Why not pass the other arguments to the constructor (*arg and atype)?

Simon