[Mesa-dev] [PATCH 1/2] clover: Factor input validation of clCompileProgram into a new function

[Francisco Jerez]

A couple of minor nit-picks below, with those fixed:

Reviewed-by: Francisco Jerez <currojerez at riseup.net>

Tom Stellard <thomas.stellard at amd.com> writes:

> This factors out the validation that is common with clBuildProgram().
> ---
>  src/gallium/state_trackers/clover/api/program.cpp | 36 ++++++++++++++++-------
>  1 file changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/api/program.cpp b/src/gallium/state_trackers/clover/api/program.cpp
> index a8a6291..bf32543 100644
> --- a/src/gallium/state_trackers/clover/api/program.cpp
> +++ b/src/gallium/state_trackers/clover/api/program.cpp
> @@ -25,6 +25,27 @@
>  
>  using namespace clover;
>  
> +namespace {
> +
Unnecessary whitespace here.

> +   void validate_build_program_common(cl_uint num_devs,
> +                                      const cl_device_id *d_devs,
> +                                      void *pfn_notify, void *user_data,

No reason to lose type information here: ^^^^^^

> +                                      const program &prog,

Can we keep the function parameters in the same order as in the original
prototype of clBuildProgram?

> +                                      ref_vector<device> devs) {

I don't think there's any reason to pass the vector of devices *and* the
d_devs array which encodes the same information.  I'd drop the vector of
devices as we don't need to error-check it when the user provides an
empty d_devs array and the CL API generates its own device list instead.

> +      if (bool(num_devs) != bool(d_devs) ||

This check will become unnecessary if you also make the change below.

> +          (!pfn_notify && user_data))
> +         throw error(CL_INVALID_VALUE);
> +
> +      if (prog.kernel_ref_count())
> +         throw error(CL_INVALID_OPERATION);
> +
> +      if (any_of([&](const device &dev) {
> +               return !count(dev, prog.context().devices());
> +            }, devs))

Use 'objs<allow_empty_tag>(d_devs, num_devs)' to get rid of the only
reference to 'devs'.

Thanks.

> +         throw error(CL_INVALID_DEVICE);
> +   }
> +}
> +
>  CLOVER_API cl_program
>  clCreateProgramWithSource(cl_context d_ctx, cl_uint count,
>                            const char **strings, const size_t *lengths,
> @@ -173,18 +194,13 @@ clCompileProgram(cl_program d_prog, cl_uint num_devs,
>     auto opts = (p_opts ? p_opts : "");
>     header_map headers;
>  
> -   if (bool(num_devs) != bool(d_devs) ||
> -       (!pfn_notify && user_data) ||
> -       bool(num_headers) != bool(header_names))
> -      throw error(CL_INVALID_VALUE);
> +   validate_build_program_common(num_devs, d_devs, (void*)pfn_notify,
> +                                 user_data, prog, devs);
>  
> -   if (any_of([&](const device &dev) {
> -            return !count(dev, prog.context().devices());
> -         }, devs))
> -      throw error(CL_INVALID_DEVICE);
> +   if (bool(num_headers) != bool(header_names))
> +      throw error(CL_INVALID_VALUE);
>  
> -   if (prog.kernel_ref_count() ||
> -       !prog.has_source)
> +   if (!prog.has_source)
>        throw error(CL_INVALID_OPERATION);
>  
>  
> -- 
> 1.8.5.5
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141023/6ee69317/attachment-0001.sig>