refactor wrapper of SimpleArray #123
Conversation
|
I think this logic, copying array, doesn't need to be inside the wrapper, the wrapper should just "wrap" things? |
cpp/modmesh/buffer/TypeBroadcast.hpp
Outdated
| * POSSIBILITY OF SUCH DAMAGE. | ||
| */ | ||
|
|
||
| #include <pybind11/pybind11.h> |
There was a problem hiding this comment.
Code using pybind11 should live in the python/ sub-directory. Otherwise, pure C++ code will include the Python.h (like here). Python.h alters a lot of things, requires to be included before everything else, and should not be included for pure C++ code.
cpp/modmesh/buffer/TypeBroadcast.cpp
Outdated
| namespace modmesh | ||
| { | ||
|
|
||
| static void TypeBroadCast<T>::check_shape(SimpleArray<T> const & arr_out, |
There was a problem hiding this comment.
This is incorrect to put template implementation in a cpp file: https://stackoverflow.com/questions/115703/storing-c-template-function-definitions-in-a-cpp-file . While forceful instantiation may be a workaround (but I do not like it), you do not do it either.
tigercosmos
changed the title
|
@yungyuc please take a look, also the failed CI is not my problem I thought. |
| namespace python | ||
| { | ||
|
|
||
| template <typename T /* original type */, typename D /* for destination type */> |
There was a problem hiding this comment.
I feel we have a better way to implement this using helper code in pybind11, but it's a good step forward to move the code to a distinct file!
There was a problem hiding this comment.
could you provide some examples about "helper code in pybind11"?
There was a problem hiding this comment.
it's a good step forward to move the code to a distinct file!
ya, now the wrapper looks more clear. :)
There was a problem hiding this comment.
@yungyuc Could you answer the question above, I am curious about it. thanks!
I just pull the master, please squash the PR when you merge it. |
|
I've rebased it. |
|
LGTM. Thanks. |
2 participants
move
TypeBroadcastoutside wrapper