search

OpenCyphal / nunavut

Generate code from DSDL using PyDSDL and Jinja2
https://nunavut.readthedocs.io/
Other
39 stars 24 forks source link

C++: Missing bool specialization for the variable-length array #281

Closed pavel-kirienko closed 1 year ago

pavel-kirienko commented 1 year ago

This is a critical issue because it makes the register API unusable with C++-generated types. For reference, in C this is supported via bitpacked arrays:

https://github.com/OpenCyphal/nunavut/blob/b3b6939dcf066e9b313bb120f2e47bb0d7bad73d/src/nunavut/lang/c/templates/definitions.j2#L139

pavel-kirienko commented 1 year ago

The specialization would offer only a subset of the functionality of the general class and use a helper reference for indexing, overall this would be similar to std::vector<bool>. There is expected to be some code duplication.

@thirtytwobits @ASMfreaK unless you have any objections I would like to go ahead and implement this now, as I am kinda stuck because of it.

asmfreak commented 1 year ago

I left this out, because we need to either:

  1. Implement our own custom bitpacked struct like the one for unions.
  2. Use external library for that. I was thinking something along the lines of this library, but it's far too big to pull it into nunavut support header.
pavel-kirienko commented 1 year ago

I see no problems with option 1. It can be implemented ad-hoc in a couple of hundred LoC. Should I proceed?

asmfreak commented 1 year ago

I have no objections. Only a minor notice. Today we have lang.variable_array_type (or smth along those lines) in the language config. Should there be a different config for bool arrays? We need @thirtytwobits opinion on this.

thirtytwobits commented 1 year ago

Implement our own custom bitpacked struct like the one for unions.

Which is this? Can you point me to what you are thinking of here?

pavel-kirienko commented 1 year ago

Should there be a different config for bool arrays?

I don't immediately see the need for it. If a specialization is necessary, it can be implemented at the language level. It's possible that I'm missing something though.

I've already basically implemented the specialization but one thing we need to know about is that we will apparently need to depend on std::allocator_traits for the sake of std::allocator_traits::rebind_alloc because the allocator for the boolean specialization is instantiated with bool while the actual storage type in this specialization is std::uint8_t. I don't see any issues here but I noticed that Scott chose to use the allocator directly instead of using the standard std::allocator_traits. Was there any reason for this other than trying to keep the implementation simpler?

@thirtytwobits @ASMfreaK