Khronos Public Bugzilla
Bug 648 - cl.hpp: constructors from refcounted C types should be explicit
Summary: cl.hpp: constructors from refcounted C types should be explicit
Alias: None
Product: OpenCL
Classification: Unclassified
Component: Header Files (show other bugs)
Version: unspecified
Hardware: All All
: P3 enhancement
Target Milestone: ---
Assignee: Aaftab Munshi
QA Contact: OpenCL Working Group
Depends on:
Reported: 2012-05-14 00:17 PDT by MattG
Modified: 2013-07-23 05:27 PDT (History)
2 users (show)

See Also:


Note You need to log in before you can comment on or make changes to this bug.
Description MattG 2012-05-14 00:17:55 PDT
This issue is best illustrated by an example.  Say I have a couple functions:

  void DoSomething(const cl::Context& context);

  void DoSomethingElse(cl::Context context);

Now, let's say I'm calling some legacy or 3rd party code which gives me a cl_context.

  cl_context CreateContext();

I want to use it to call DoSomething(), followed by DoSomethingElse().

  cl_context my_context = CreateContext();  // refcount: 1
  DoSomething(my_context);                  // refcount: 0
  DoSomethingElse(my_context);              // boom!

What happened?  Well, whether by const ref or by value, DoSomething() and DoSomethingElse() both take a cl::Context.  cl::Context has a constructor:

  cl::Context::Context(const cl_context&);

which invokes:

  cl::detail::Wrapper<cl_context>::Wrapper(const cl_context &)

This constructor, unlike Wrapper's copy constructor, doesn't call retain() on the new value.  Yet, ~Wrapper() always calls release() (if non-NULL).  I think there are arguments to be made both for and against the constructor calling retain().  However, the net effect of not calling it is that each time one invokes this constructor, a refcount is transferred into the Wrapper.

Given their refcount-consuming behavior, you want the caller to be aware of any temporaries costructed by this means.  To this end, C++ has provided the explicit keyword.

Unfortunately, using explicit would introduce a compilation error in the above code.  In order for it to compile (without changing its behavior), it would need to look like this:

  cl_context my_context = CreateContext();  // refcount: 1
  DoSomething(cl::Context(my_context));     // refcount: 0
  DoSomethingElse(cl::Context(my_context)); // boom!

The code is still wrong, but hopefully the problem is more obvious.  Also, in order to save typing, making the constructor explicit might encourage writing it as:

  cl::Context myContext(CreateContext());  // refcount: 1
  DoSomething(myContext);                  // refcount: 1
  DoSomethingElse(myContext);              // refcount: 1
                                           // refcount: 0

which fixes the problem.

So, either change the refcounting behavior of the C-type constructors (which I don't recommend), or make them explicit.  However, because the 'explicit' change can break existing code, it should be encapsulated in a macro (CL_EXPLICIT?) that can be disabled.
Comment 1 Lee Howes 2012-05-17 15:44:40 PDT
The only advantage of the macro would be that *when* code breaks defining one macro is all that is necessary, not putting in a whole pile of casts, yes?
Comment 2 MattG 2012-05-27 10:11:47 PDT
(In reply to comment #1)
> The only advantage of the macro would be that *when* code breaks defining one
> macro is all that is necessary, not putting in a whole pile of casts, yes?

My rationale for recommending the macro is that the person building the code is often someone other than the author.  I think it's generally easier to explain to someone how to add a macro definition to the compiler flags than to expect them to go and fix all of the compilation failures in the code.

The problem with changing the refcounting behavior of the constructors is that it would break code which relies the wrappers' current behavior to support RAII:

  cl::Context myContext(CreateContext());

If the wrapper were to increment the refcount, it would create leaks in existing code and force users to find/create another mechanism to tie that refcount to a scope.

I think 'explicit' is a good compromise.  Wrapping it with a macro is easy and should help existing users cope with the change.
Comment 3 Bruce Merry 2012-06-15 07:51:23 PDT
I agree that adding the 'explicit' keyword is a better fix than changing the refcounting behaviour: breakage of existing code will be reasonably obvious and at compile time, rather than by introduction of resource leaks.

A macro as a way for people to easily keep old code working again sounds good too, although I'd give it a name that describes exactly what it does rather than just CL_EXPLICIT (the latter sounds like it wraps *all* use of the keyword). Maybe something like CL_EXPLICIT_WRAPPER_FROM_C.
Comment 4 Bruce Merry 2013-07-23 05:27:06 PDT
The macro solution has been shipping for a while now, so closing.