-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add the possibility to store channels of different types #168
Conversation
- Fixed a typo - Align multiple types test to the README.
This depends on the latest version of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very nice addition, thanks a lot @S-Dafarra!
Only some small questions, in particular about how this implementation works.
Moreover, could you please add a line in the CHANGELOG? Thanks!
} | ||
|
||
private: | ||
size_t m_payload{0}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The payload information wasn't used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was used in some examples using directly the Record
struct, but never in BufferManager
, which is basically the only interface used by downstream projects. In fact, when pushing to a buffer, we were passing directly the vector, and not a Record
. Hence, that information was not accessible anyway.
I had to remove it because it was not clear how to define the payload when using std::any
.
|
||
// Trying to apply the rule of zero | ||
|
||
std::any m_datum;/**< the actual data of the record */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you removed all the constructors for let the compiler generate the default ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically yes. Being a simple struct with two fields (having removed m_payload
), it is enough to use the direct-list-initialization, and a dedicated constructor was not necessary.
} | ||
|
||
// matiomatioCppCanConcatenate<T>::value is true when T has the T::value_type member. If this is true, then we check | ||
// if T is either an Element, a Vector (but not a String), or a MultidimensionalArray |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the usage of this struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used to understand how to "concatenate" the data stored for each time instant. In the case of n-dimensional numeric entries like scalars, vectors, and multi-dimensional arrays, it is possible to concatenate in an n+1 dimensional array. On the other hand, with complex inputs like strings or structs, it is possible to concatenate only in a cell array, or in a struct array. That struct uses SFINAE to detect if we can concatenate in an n+1 dimensional array or not.
I tried to not use SFINAE, but the problem was that I needed to check if the matioCppType::type
member existed in matioCppType
(being matioCppType
the matioCpp
type corresponding to the input type T
), and this was not possible using if constexpr
.
std::mutex m_buff_mutex; | ||
dimensions_t m_dimensions; | ||
size_t m_dimensions_factorial{0}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain the usage of this m_dimensions_factorial
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It stores the result of the product of all the dimensions in m_dimensions
.
} | ||
|
||
template<typename T> | ||
void createMatioCppConvertFunction() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a few lines of documentation about this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in a536901. Let me know if it is ok
Edited the CHANGELOG.
Done in a536901 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR and the explanations, is much more clear now!
Merged, thanks! |
No description provided.