Khronos Public Bugzilla
Bug 252 - Feedback on OpenCL C++ bindings: size_t<n> type misleading.
Feedback on OpenCL C++ bindings: size_t<n> type misleading.
Status: RESOLVED FIXED
Product: OpenCL
Classification: Unclassified
Component: Header Files
1.0
PC Linux
: P3 normal
: ---
Assigned To: Benedict Gaster
OpenCL Working Group
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-22 00:25 PST by Terence Smith
Modified: 2013-07-23 05:38 PDT (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Terence Smith 2010-01-22 00:25:26 PST
overview:

Current size_t<n> constructor is an empty cl::vector.  This is misleading as the natural reading of size_t<n> is an n-tuple that can be subscripted into.  

Currently, inserting values by subscripting will place the values given into the statically allocated internal array, but the vector is still marked as empty and zero length.  As such, attempting to copy the object will copy none of the values across to the new vector.  Worse, since internal functions (i.e. the Image2D constructor) don't check whether the vector is empty, but rather access the internal array directly, these internal functions will succeed while a simple copy via the provided copy constructor will silently fail.

Steps to Reproduce:
cl::size_t<3> imSize;
imSize[0] = 1280;
imSize[1] = 1920;
imSize[2] = 0;
std::cout << "imSize:" << imSize[0] << std::endl;
cl::size_t<3> test(imSize);
std::cout << "test:" << test[0] << std::endl;

Actual Results:
imSize:1280
test:27553456 (this is just a random uninitialized value)

Expected Results:
imSize:1280
test:1280

Platform:
Linux, cl.hpp version 0.3, date July 2009

a patch for a simple fix is below, just replacing the default vector constructor
with the constructor to initialise N values. (though this will break code that relies on the vector being empty)

777,781c777
< struct size_t : public cl::vector< ::size_t, N> {
< public:
< 	size_t() : cl::vector< ::size_t, N>( N ) {};
< 
< };
---
> struct size_t : public cl::vector< ::size_t, N> { };
Comment 1 Sebastian Schuberth 2011-01-07 06:20:01 PST
I believe there are more underlying problems:

Strictly speaking, you're using cl::size_t in the wrong way, because cl::size_t<3> creates a vector with a capacity of 3, not a size of 3. Well, that was if capacity() was implemented correctly, as it currently returns the number of bytes, not elements, the vector can hold without reallocation.

Anyway, a proper way to fill cl::size_t<3> would be by calling push_back(). That way, _empty is not set to true anymore.

But I agree the way you're using cl::size_t<3> is very natural, and it should either work or not compile.

It seems there are more problems with cl::size_t / cl::vector, e.g. the destructor does not call the destructors on its element (basically, it should call pop_back() for all elements). Also, having _empty at all and setting size to -1 instead of 0 for an empty vector seems to be a little heavy-handed to me. IMHO that class should undergo a through review / redesign.
Comment 2 Benedict Gaster 2011-01-07 12:18:27 PST
I like the size_t<> usage but do completely agree that cl::vector needs a strong review and really needs a complete rewrite. At this time I am not going to be able to get to this for the next month or so (at least).
Comment 3 Bruce Merry 2013-07-23 05:38:17 PDT
It looks like cl::size_t has been rewritten as a separate class from cl::vector, so closing.