-
Notifications
You must be signed in to change notification settings - Fork 0
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
zserio::Array is cumbersome to use #67
Comments
Thanks for a comprehensive description and for sharing the ideas. First of all, your last note is correct.
This is necessary for the indexed offsets. Offsets are evil. We need to have possibility to set elements in the array used as an indexed offset array. Therefore, we need to support mutable arrays as well which must hold non-const reference to the underlying array. Currently, indexed offsets arrays are implemented as
If you need to work with all
We agree that template arguments are annoying. We were not able to find out better solution for that. We could probably move them to the runtime as the constructor arguments. But this will bring some performance drop together with bigger memory allocations.
If we change template argument |
I don't want to copy anything. I want to write a function that works on an
Now I have to potentially spell out multiple verbose template arguments, plus the underlying storage. Whereas I just want to work over a sequence of elements of type T, so I just want to type
See my previous point: for methods, that's an issue.
But For the sake of argument, let's re-imagine
|
Array can probably hold For setting of indexed offsets we need to know parameters since the offset expression can contain parameters - e.g. offset field lies in compound provided as a parameter. So we need some kind of
I understand that it's not nice to specify the full type manually, but since you will work with
It seems weird to me to have implementation for one specific type in a source file (cpp). In all cases we were not able to find out better solution for template arguments as Miki wrote above.
Note that even now you should be able to use just |
Another idea is that you could use your own typedef for the array type, something like:
|
And what if we generate Arrays? Could keep ArrayBase (with the span) generic.
Right, and this last bit creates the problem: two arrays with the same
As I mentioned, using templates whenever ANY Array is needed isn't nice for compilation time, readability, and doesn't work well if the code is split over header/source.
Not really. Want to convert an Array of Position2Ds to some properietary Line2D type? That could be a utility method. Or some member of a class that needs to work on an Array, but also on member functions, then it needs to be a private method declared in the header, while the definition can be in the source file.
Sure, or do
I understand capacity is an issue, and we could leave this as-is for now since there might be more important things to work on, but I would prefer this is changed before actual release.
Yes, but it's all overhead. It feels like it shouldn't be necessary. I'd just like to explore the alternatives. The point of this new generator is also to make the using code easier to write and maintain.
Why? It's just to get access to parameters to construct a View, right? If you generate Arrays like Views, that's not necessary anymore? |
The generation of the arrays is, of course, possible. However, this would present additional problems:
In all cases, such discussions are highly valuable because we are able to find out together better technical solution. Because we cannot do everything immediately, I suggest the following approach:
|
I'd like to start a discussion about
zserio::Array
's ease of use. It takes several template parameters:std::vector<T>
).This is quite cumbersome to use. For example, if I have a compound type with two arrays, and I want to write a function that returns either of the arrays for a given object as a reference (because I don't want to pay for a copy), I have to specify quite a full type, e.g.:
This gets even more complex if the storage isn't immutable.
Instead, I'd prefer to be able to write something simple like this:
I believe this should be possible in principle, because
ArrayType
is only relevant for serialization. Perhaps this can be removed from the Array type, and passed as harcoded argument when writing. It's really something that users of the API shouldn't have to care about.Similarly, at first glance,
ArrayStorage
seems weird to have. Why is having aconst Array<...>&
not good enough? Why must theArray
itself know whether it's mutable or not?And since
ARRAY_TRAITS
is taken from a specialization ofzserio::ArrayTraits<T>
anyway, why bother having it as a template argument at all?Edit: about ARRAY_TRAITS, I see now that it's not always from
zserio::ArrayTraits
. It's possible to have two arrays with the same value type, but have different traits because they come from e.g. different members of a compound struct. This means I cannot use the same code to handle them both, unless I template it. I'd still like it if this array traits were somehow not a part of the type.Lastly, is there a use case for having something other than
std::vector
as storage? Perhaps if we wish to keep supporting this, we can change the definition to the following, to simplify the common case:Edit: I also see that
Array
doesn't own its storage, so perhapsArrayView
orSpan
is a better name, for consistency and clarity? i.e.ArrayView<SomeType>
(also known as aSpan
, in which case a begin/end iterator is enough, and the underlying storage isn't needed at all?)The text was updated successfully, but these errors were encountered: