[Mesa-dev] [PATCH 08/14] glsl: Add copy-constructor for ast_struct_specifier.

[Chad Versace]

On 07/03/2013 12:03 PM, Matt Turner wrote:
> ---
>   src/glsl/ast.h | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> diff --git a/src/glsl/ast.h b/src/glsl/ast.h
> index 3bb33c5..f00ef3a 100644
> --- a/src/glsl/ast.h
> +++ b/src/glsl/ast.h
> @@ -453,6 +453,18 @@ class ast_declarator_list;
>
>   class ast_struct_specifier : public ast_node {
>   public:
> +   /**
> +    * \brief Make a shallow copy of an ast_struct_specifier.
> +    *
> +    * Use only if the objects are allocated from the same context and will not
> +    * be modified. Zeros the inherited ast_node's fields.
> +    */
> +   ast_struct_specifier(const ast_struct_specifier *that):
> +      ast_node(), name(that->name), declarations(that->declarations)
> +   {
> +      /* empty */
> +   }
> +
>      ast_struct_specifier(const char *identifier,
>   			ast_declarator_list *declarator_list);
>      virtual void print(void) const;


See patch 7's review. This isn't a copy constructor. The copy constructor's
signature is T::T(const T&) where T=ast_struct_specifier.

The compiler still generates the default copy constructor, so we get this scary
behavior:

    ast_struct_specifier orig;
    ... // do stuff to orig

    // This creates the copy you want.
    ast_struct_specifier copy0(&orig);
    assert(copy0.link.next == NULL);

    // If someone accidentally writes this code, it compiles without warning,
    // but creates a buggy copy.
    ast_struct_specifier copy1(orig);
    assert(copy0.link.next == orig.link.next);

There are two ways to safely fix this patch. A C++-way that uses
references, and a way that lets people code in the C-way using pointers.

Fix 1
-----
Change the signature to `ast_struct_specifier(const ast_struct_specifier& that)`.
Then you will have defined the copy ctor, preventing the generation of the default
copy constructor. A possible unwanted side-effect, from the perspective of a C
programmer, is that we must pass the original ast_struct_specifier as a ref rather
than a pointer.

Fix 2
-----
Leave the constructor as-is in the patch. Fix the commit subject as discussed in
patch 7's review, since the patch now adds a new constructor, not a copy
constructor. Then, to prevent accidental use of the default copy constructor,
prevent it's generation by declaring it but not providing a definition.
It doesn't matter if you declare it as public or private, since it will
cause a compile-time error either way if anyone tries to use it,
but Google's and LLVM's practice is to declare it private.

Like this:

    class ast_struct_specifier: public ast_node {
       ...
    private:
       /* Prevent use of copy constructor. */
       ast_struct_specifier(const ast_struct_specifier&);
};

----

I prefer 1 because, if we're defining a ctor that looks and acts like the
copy ctor, then let's just define the copy ctor and be done with it.
However, I don't know how you're using this in future patches, so it's
your call.