srt icon indicating copy to clipboard operation
srt copied to clipboard

[core] Remove use variable length array

Open quink-black opened this issue 4 years ago • 4 comments

quink-black avatar Apr 05 '22 14:04 quink-black

I would propose using the FixedArray then (from utilities.h). It can't (and is not expected to) be resized, unlike std::vector. Even though in this case of a local variable it is not that important.

maxsharabayko avatar Apr 05 '22 15:04 maxsharabayko

I would propose using the FixedArray then (from utilities.h). It can't (and is not expected to) be resized, unlike std::vector. Even though in this case of a local variable it is not that important.

Could you give some hints about the benefits of FixedArray other than forbidden of resize? There is no reallocation here, so I guess the overhead of std::vector can be negligible.

quink-black avatar Apr 06 '22 03:04 quink-black

Could you give some hints about the benefits of FixedArray other than forbidden of resize? There is no reallocation here, so I guess the overhead of std::vector can be negligible.

It is a perfectly fair question, but I have no unambiguously correct answer to it. I will give my thoughts, while I do not insist on using the FixedArray. Especially given we talk about a local variable with a short lifetime.

In this very place, we want to have an array of epoll_event. The ideal would be to use std::array, except we don't know the size at compile time. Therefore we need an array allocated in run-time, preferably destructing itself properly, so we don't need to call delete []. Based on this description a conventional guideline is to use std::vector.

However, std::vector is much more powerful than just a dynamically allocated array. Thus we get two disadvantages:

  1. If a non-const std::vector is used, we don't have a guarantee that somewhere it will not be resized. The use of vector itself does not provide this intent to the reader of the code. In this case of a local scope - not a big deal.
  2. Slightly extra footprint. We need a pointer and array size, we get a bit more. E.g. see the below implementation of the std::vector having two-pointers and an additional capacity pointer. (The FixedArray currently also has an extra pointer pointing to an error string I brought in PR #2184. A quick and dirty fix to be re-worked.)

Thus in this "proposal" my main motivation is the first item from the list above: to show intent it is just an array we don't plan to resize.

template <class _Tp, class _Allocator>
class __vector_base
    : protected __vector_base_common<true>
{
    // ...
    pointer                                         __begin_;
    pointer                                         __end_;
    __compressed_pair<pointer, allocator_type> __end_cap_;
    // ...
}

maxsharabayko avatar Apr 06 '22 09:04 maxsharabayko

PR #2298 adds srt::FixedArray::data() that (once merged) can be used here instead of &ev[0].

maxsharabayko avatar Apr 14 '22 16:04 maxsharabayko