Skip to content

Comments

Add empty checks#339

Open
bashbaug wants to merge 3 commits intoKhronosGroup:mainfrom
bashbaug:add-empty-checks
Open

Add empty checks#339
bashbaug wants to merge 3 commits intoKhronosGroup:mainfrom
bashbaug:add-empty-checks

Conversation

@bashbaug
Copy link
Contributor

fixes #338 by explicitly passing nullptr when a vector is empty.

There were a lot of these... I think I got them all.

I added a bunch of unit tests when related tests existed, but there are still some unit testing gaps. We're no worse off than we were previously, though.

@bashbaug
Copy link
Contributor Author

@jansol, mind taking a look?

Copy link
Contributor

@jansol jansol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

For consistency the various calls to clCreateImageWithProperties and clCreateBufferWithProperties could be switched to using the ternary operator instead of two separate calls in an if-else clause, but functionally they are fine as is.

Comment on lines +7222 to +7225
vector<cl_device_id> deviceIDs;
for(const Device& device: devices) {
deviceIDs.push_back(device());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the idea that the in-memory representation of the wrapper class objects is == the underlying handles? From a quick look my understanding is that this should be the case for Device too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the general idea, yes, but device objects are an exception since they have an extra variable:

template <>
class Wrapper<cl_device_id>
{
public:
    typedef cl_device_id cl_type;

protected:
    cl_type object_;
    bool referenceCountable_;
...

There might be a better way to do this, but I think it's outside of the scope of this PR...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed that 2nd field. Yea that's a whole separate topic then.

@bashbaug
Copy link
Contributor Author

For consistency the various calls to clCreateImageWithProperties and clCreateBufferWithProperties could be switched to using the ternary operator instead of two separate calls in an if-else clause, but functionally they are fine as is.

May as well fix this while we're at it. Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Calling setSVMPointers with an empty list causes incorrect behaviour

2 participants