[Mesa-dev] [PATCH 09/11] clover: Add constructors to some of the module classes

[Francisco Jerez]

Hi Tom, thanks for the patches, just a few suggestions inline.

Tom Stellard <tstellar at gmail.com> writes:

> This is for the llvm code that can't use extended initializers.
> ---
>  src/gallium/state_trackers/clover/core/module.hpp |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/core/module.hpp b/src/gallium/state_trackers/clover/core/module.hpp
> index bc4b203..f7f7bb0 100644
> --- a/src/gallium/state_trackers/clover/core/module.hpp
> +++ b/src/gallium/state_trackers/clover/core/module.hpp
> @@ -48,6 +48,11 @@ namespace clover {
>           type type;
>           size_t size;
>           clover::compat::vector<char> data;
> +
> +         section(resource_id id, enum type type, size_t size,
> +                 clover::compat::vector<char> data) :
> +                 id(id), type(type), size(size), data(data) { }
> +         section() { }
>        };
>  
The "data" argument should be declared as a const reference to avoid an
additional copy.

>        struct argument {
> @@ -65,6 +70,9 @@ namespace clover {
>  
>           type type;
>           size_t size;
> +
> +         argument(enum type type, size_t size) : type(type), size(size) { }
> +         argument() { }
>        };
>  
>        struct symbol {
> @@ -72,6 +80,11 @@ namespace clover {
g>           resource_id section;
>           size_t offset;
>           clover::compat::vector<argument> args;
> +
> +         symbol(clover::compat::vector<char> name, resource_id section,
> +                size_t offset, clover::compat::vector<argument> args) :

Same here, "name" and "args" should probably be const references.

> +                name(name), section(section), offset(offset), args(args) { }
> +         symbol() { }

I think we want to initialize the scalar members of all the three
structs to sensible values in their default constructors, otherwise
they'll be left uninitialized.

And BTW, for consistency with the rest of the code, can you define the
constructors before the data members in each struct declaration instead
of at the bottom?

>        };
>  
>        void serialize(compat::ostream &os) const;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20120518/23c413c3/attachment.pgp>