Fix naming inconsistency khr #244
Conversation
There was a problem hiding this comment.
Thanks, this looks like the right direction, but the inconsistency still exists because now the CommandBuffer and MutableCommandBuffer classes are in the khr namespace, but the Semaphore class is not. Are you planning to move the Semaphore class into the khr namespace also?
As an aside, it looks like parts of this file were reformatted but had no other changes. I would generally advise against this, but if would like to reformat parts of the file could you please do it as the first or last commit? This will make it easier to see and review the non-formatting changes - thanks!
|
Hi @bashbaug |
|
Related to #233 |
There was a problem hiding this comment.
Thanks, this mostly looks good to me, thanks for making these changes.
I have one specific comment for the test changes, see below.
Additionally, I'd like to reiterate that formatting related changes make it much harder to review this PR than it should be. I'd strongly recommend reverting the formatting changes to minimize diffs.
Finally it appears that this PR needs a rebase since there are merge conflicts.
tests/test_openclhpp.cpp
Outdated
| namespace cl{ | ||
| MAKE_MOVE_TESTS(Context, make_context, clReleaseContext, contextPool) | ||
|
|
||
| } |
There was a problem hiding this comment.
This is a little ugly. How about this as an alternative:
- Separate out the "type" and the "typestr", whereas we just have a single "type" currently.
- Use
Contextas the "typestr" in this case, and e.g.CommandBufferas the "typestr" in the command buffer cases. - Use
cl::Contextas the "type" in this case, and e.g.cl::khr::CommandBufferas the "type" in the command buffer cases.
I think this will require fewer diffs, and it won't require putting the tests themselves inside of the cl or cl::khr namespaces, which they really shouldn't be doing.
|
|
kamil-goras-mobica
force-pushed
the
fix_naming_inconsistency_khr
branch
3 times, most recently
from
139167b to
f98dfed
Compare
kamil-goras-mobica
force-pushed
the
fix_naming_inconsistency_khr
branch
from
f98dfed to
73c5491
Compare
There was a problem hiding this comment.
Apologies for the long review delay. The updated change LGTM. There needs to be a rebase (in the test code?) now, but hopefully that is straightforward.
Noting for completeness: these changes do break backward compatibility so they will require code changes for applications using the C++ bindings with semaphores and command buffers.
No description provided.