search

KhronosGroup / OpenCL-CLHPP

Khronos OpenCL-CLHPP
Apache License 2.0
375 stars 130 forks source link

Added wrapper constructors for clCreateImageWithProperties #278

Closed shajder closed 3 months ago

shajder commented 1 year ago

Added new constructors for Image1D, Image1DArray, Image1DBuffer, Image2D, Image2DArray, Image3D to wrap clCreateImageWithProperties features.

I have tested/discussed most of new constructors in real environment, here is modified juliavk sample (samples/vulkan/00_juliavk) adapted from @bashbaug repository to test new constructors.

shajder commented 3 months ago

LGTM, this seems like a useful addition.

     *  \param properties Optional list of properties for the image object and
     *                    their corresponding values. The non-empty list must
     *                    end with 0.

Is it desirable to detect cases when the list of properties does NOT end with zero and return some kind of error?

Sounds good for me but ... in such case we should probably do the same in other locations eg. Buffer class, right ?

bashbaug commented 3 months ago

Sounds good for me but ... in such case we should probably do the same in other locations eg. Buffer class, right ?

Ah, good point. Let's save this for a later PR, then.

Last question for this PR: would it be possible to add some tests for this new functionality, similar to testBufferWithProperties?

shajder commented 3 months ago

would it be possible to add some tests for this new functionality, similar to testBufferWithProperties?

sure thing, are you thinking about this PR or separate ?

bashbaug commented 3 months ago

sure thing, are you thinking about this PR or separate ?

In this PR would be ideal, then it doesn't add to the backlog of unit test PRs we still need to review and merge.

Is this doable? If not, we'll merge now, and add it to the list (risk seems small).

shajder commented 3 months ago

Is this doable?

I will complement shortly and let you know

shajder commented 3 months ago

would it be possible to add some tests for this new functionality

@bashbaug I just added tests for Image2D and Image3D. What is missing is the rest of Image constructors, not only new but any tests for Image1D, Image1DArray, Image2DArray, worth creating an issue ?

bashbaug commented 3 months ago

Thanks, tests LGTM. The CI failures are unrelated to your change, so I'll just merge this.