Static vector

[grauw]

Hi Wouter,

Here’s a spin-off from pull request #1620 where I expand the implementation of static_vector, add serialization support and use it to store the AmdFlash command list. Also already adds a dummy “status” field for use later.

[grauw]

One open question; can I forward elem here or not?

	template<typename Range>
	constexpr static_vector(from_range_t, Range&& range) {
		for (auto&& elem : range) {
-			push_back(elem);
+			push_back(std::forward<T>(elem));
		}
	}

Originally I thought I could std::move(elem) directly, since && variables are always an rvalue (I looked that up to double check since I wasn’t sure). But then Sonarcloud complained and I found that actually if Type is a template parameter or auto variable then it’s not always an rvalue (thank you C++) so I should use std::forward. But Sonarcloud also tells me that if T is a class template parameter it is not forwardable, and the type of elem would be T, so now I’m immensely confused.

Then I thought maybe I should do std::forward<Range>(elem) since it depends on whether Range is an rvalue, but that fails to compile. Maybe I can still do std::forward<T>(elem), but maybe that’s a no-op? I gave up and went back to the original implementation which just copies the damn thing. Maybe your C++-fu is stronger than mine 🙂.

[m9710797]

One open question; can I forward elem here or not?

This is indeed a bit tricky. And what makes this even more difficult is that Sonarcloud generates some false-positive warnings in this area. Usually I just follow some rule-of-thumbs and then stuff just works. But since you ask I'll try to reconstruct the underlying C++ rules (and I learned some new things myself while doing this).

There are two separate issues. How to pass the range function parameter. How to pass the elem variable to push_back().

• Rule of thumb: pass ranges by r-value.

Unfortunately Sonarcloud does not agree with this rule. But I double checked this with the advice given in some c++ book I read (I think it was https://cppstd20.com/ the section about ranges and views).
The reason is that in general a range can caches internal state, and then passing by const-reference won't compile. For example calling begin() on a views::filter may do some non-trivial amount of work, that's cached for the next time begin() is called. Passing by value would be an option for most views (views are typically cheap to copy), but that's not a good option when the range is a full container. Passing as an r-value works for both cases.

• How to forward elem?

Using a variable by-name always gives an l-value. With std::move() you can unconditionally cast this to an r-value. With std::forward() you cast either to an l- or an r-value, depending on the "original" value-category (l/r-value) of the type. But in order to do this std::forward() needs extra information, namely the original type of that variable. This information is passed as a template parameter to std::forward().

So in this specific example we need to know the type of the elem variable. In the code that type 's written as auto&& (you can think of this as a new unnamed template type). So we cannot directly name that type. Instead we can obtain it via decltype(elem). The full statement then becomes:

push_back(std::forward<decltype(elem)>(elem));

Feel free to make this change in the code.
This pattern std::forward<decltype(X)>(X) is a common idiom, and some people write a macro to shorten it (but personally I don't like that).

Here is the experiment I did to verify some of the above information: https://godbolt.org/z/az1zxY1vr
I found out that "moving" ranges is still a bit more complicated than moving "other" types. (E.g. sometimes you have to use std::views::as_rvalue to really move the elements out of a range).

[m9710797]

static_vector: Expand implementation.

Looks good.

I assume you need (some of) this in a later patch? Possible because you use it in combination with view::take()?

Background info:
The intention is that in the future we replace our static_vector with a c++ standard implementation. The proposal is being discussed here: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p0843r11.html (the original name was also static_vector, later it was renamed to inplace_vector).

[grauw]

I assume you need (some of) this in a later patch? Possible because you use it in combination with view::take()?

The serialisation needed using value_type = T and data(). In the other patch series I make use of max_size() and back(). I also wanted to use emplace_back() but couldn’t fully (see comment in 3rd patch). The rest is me just padding out the implementation.

Background info: The intention is that in the future we replace our static_vector with a c++ standard implementation. The proposal is being discussed here: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2024/p0843r11.html (the original name was also static_vector, later it was renamed to inplace_vector).

Would be great to have in the standard library indeed.

[m9710797]

serialize_stl: Support static_vector serialisation.

Loading has a bug, see below.

[m9710797]

AmdFlash: Use static_vector to store command list.

Only some comments about small tweaks or suggestions (feel free to ignore).
And one comment about loading status in case of an old savestate.

[grauw]

Review comments addressed, and issue fixed in back() found during testing.

[m9710797]

Since you mentioned a bug fix in this class (which I overlooked in my first review), I did a 2nd review. I found another bug, but maybe we shouldn't fix it? (instead we can accept a limitation).