A couple of minor nit-picks below, with those fixed: Reviewed-by: Francisco Jerez <[email protected]>
Tom Stellard <[email protected]> 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
pgpHZadyzZPBZ.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
