Re: [PATCH 2/2] m4: generate a basic_symbol constructor for each symbol type

[Akim Demaille]

Le 28 janv. 2013 à 20:29, Theophile Ranquet <address@hidden> a écrit :

> Recently, there was a slightly vicious bug hidden in the make_ functions:
> 
>  parser::symbol_type
>  parser::make_TEXT (const ::std::string& v)
>  {
>    return symbol_type (token::TOK_TEXT, v);
>  }
> 
> The constructor for symbol_type doesn't take an ::std::string& as
> argument, but a constant variant.  However, because there is a variant
> constructor which takes an ::std::string&, this caused the implicit
> construction of a built variant.  Considering that the variant argument
> for the symbol_type constructor was cv-qualified, this temporary variant
> was never destroyed.
> 
> As a temporay solution, the symbol was built in two stages:

Temporary.

> 
>  symbol_type res (token::TOK_TEXT);
>  res.value.build< ::std::string&> (v);
>  return res;
> 
> However, the solution introduced in this patch contributes to letting
> the symbols handle themselves, by supplying them with constructors which

I'd go for "that" here.

> take a non-variant value and build the symbol's own variant with that
> value.
> 
> * data/variant.hh (b4_symbol_constructor_define_): Use the new
> constructors rather than building in a temporary symbol.
> (b4_basic_symbol_constructor_declare,
> b4_basic_symbol_constructor_define): New, macros generating the

Remove the comma.

> constructors.
> * data/c++.m4 (basic_symbol): Invoke the macros here.
> ---
> data/c++.m4     | 12 +++++++++---
> data/variant.hh | 38 +++++++++++++++++++++++++++++++++-----
> 2 files changed, 42 insertions(+), 8 deletions(-)
> 
> diff --git a/data/c++.m4 b/data/c++.m4
> index d134eb8..d9f86cd 100644
> --- a/data/c++.m4
> +++ b/data/c++.m4
> @@ -174,10 +174,12 @@ m4_define([b4_public_types_declare],
> 
>       /// Copy constructor.
>       basic_symbol (const basic_symbol& other);
> -
> +]b4_variant_if([[
> +      /// Constructor for valueless symbols, and symbols from each type.
> +]b4_type_foreach([b4_basic_symbol_constructor_declare])], [[
>       /// Constructor for valueless symbols.
>       basic_symbol (typename Base::value_type t]b4_locations_if([,
> -                    const location_type& l])[);
> +                    const location_type& l])[);]])[
> 
>       /// Constructor for symbols with semantic value.
>       basic_symbol (typename Base::value_type t,
> @@ -279,6 +281,10 @@ m4_define([b4_public_types_define],
>     (void) v;
>     ]b4_symbol_variant([this->type_get ()], [value], [copy], [v])])[}
> 
> +]b4_variant_if([[
> +  // Implementation of basic_symbol constructor for each type.
> +]b4_type_foreach([b4_basic_symbol_constructor_define])], [[
> +  /// Constructor for valueless symbols.
>   template <typename Base>
>   inline
>   ]b4_parser_class_name[::basic_symbol<Base>::basic_symbol (]b4_join(
> @@ -287,7 +293,7 @@ m4_define([b4_public_types_define],
>     : Base (t)
>     , value ()]b4_locations_if([
>     , location (l)])[
> -  {}
> +  {}]])[
> 
>   template <typename Base>
>   inline
> diff --git a/data/variant.hh b/data/variant.hh
> index 84ea779..0c9eec5 100644
> --- a/data/variant.hh
> +++ b/data/variant.hh
> @@ -311,15 +311,43 @@ m4_define([b4_symbol_constructor_define_],
>   b4_parser_class_name::make_[]b4_symbol_([$1], [id]) (dnl
> b4_join(b4_symbol_if([$1], [has_type],
>                      [const b4_symbol([$1], [type])& v]),
> -        b4_locations_if([const location_type& l])))[
> +        b4_locations_if([const location_type& l])))
>   {
> -    symbol_type res (token::]b4_symbol([$1], [id])[]b4_locations_if([, l])[);
> -    ]b4_symbol_if([$1], [has_type], [res.value.build (v);
> -    ])[return res;
> +    return symbol_type (b4_join([token::b4_symbol([$1], [id])],
> +                                b4_symbol_if([$1], [has_type], [v]),
> +                                b4_locations_if([l])));
> +
>   }
> 
> -]])])])
> +])])])
> +
> 
> +# b4_basic_symbol_constructor_declare
> +# ----------------------------------
> +# Generate a constructor declaration for basic_symbol from given type.
> +m4_define([b4_basic_symbol_constructor_declare],
> +[[
> +  basic_symbol (]b4_join(
> +          [typename Base::value_type t],
> +          b4_symbol_if([$1], [has_type], const b4_symbol([$1], [type])[ v]),
> +          b4_locations_if([const location_type& l]))[);
> +]])

So you really handle this at the basic_symbol level.  Why not,
Indeed.  Fine, please install!

> +# b4_basic_symbol_constructor_define
> +# ----------------------------------
> +# Generate a constructor implementation for basic_symbol from given type.
> +m4_define([b4_basic_symbol_constructor_define],
> +[[
> +  template <typename Base>
> +  ]b4_parser_class_name[::basic_symbol<Base>::basic_symbol (]b4_join(
> +          [typename Base::value_type t],
> +          b4_symbol_if([$1], [has_type], const b4_symbol([$1], [type])[ v]),
> +          b4_locations_if([const location_type& l]))[)
> +    : Base (t)
> +    , value (]b4_symbol_if([$1], [has_type], [v])[)]b4_locations_if([
> +    , location (l)])[
> +  {}
> +]])
> 
> # b4_symbol_constructor_define
> # ----------------------------
> -- 
> 1.8.1.1
> 
>