Update for removal of externref from C API#48
Conversation
Also update some utilities on `Val` which can no longer be implemented without a store parameter.
| TrapResult<std::vector<Val>> call(Store::Context cx, | ||
| const std::vector<Val> ¶ms) const { | ||
| return this->call(cx, params.begin(), params.end()); | ||
| } | ||
|
|
||
| TrapResult<std::vector<Val>> call(Store::Context cx, | ||
| const std::initializer_list<Val> ¶ms) const { | ||
| return this->call(cx, params.begin(), params.end()); | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't know what version of C++ is supported here (you might be able to leverage ranges), but generally I would expect a single templatized version like so:
template<typename C>
TrapResult<std::vector<Val>> call(Store::Context cx,
C &¶ms) const {
return this->call(cx, std::begin(params), std::end(params));
}Inspiration you can take is from Google's absl library container algorithms: https://github.com/abseil/abseil-cpp/blob/74df6975aef6d6fa2020313922e9a94b42364f38/absl/algorithm/container.h#L191-L200
That also makes it compatible C++20 ranges: https://en.cppreference.com/w/cpp/ranges
Which then you could constrain to a concept if needed:
TrapResult<std::vector<Val>> call(Store::Context cx,
std::ranges::input_range auto &¶ms) const {
return this->call(cx, std::begin(params), std::end(params));
}There was a problem hiding this comment.
Oh nice, thanks!
Is there a way to influence via the typename/template parameter what the collection should contain? If I replaced the two call overloads added in this PR with your suggestion it fails for {} as it can't infer the type of the list and it also fails for {1} as it doesn't infer that it should be Val (I think)
There was a problem hiding this comment.
I don't think there is a way to do this without the initializer list overload. At least not with concepts: https://godbolt.org/z/c91sancY5. Maybe there is some SFINAE magic here somehow?
There was a problem hiding this comment.
Ah ok, no worries then! I'll leave it as-is and see if folks have issues in that case. (I barely know what SFINAE is myself other than that it's a thing)
| for (const auto ¶m : params) { | ||
| raw_params.push_back(param.val); | ||
| raw_params.reserve(end - begin); | ||
| for (auto i = begin; i != end; i++) { |
There was a problem hiding this comment.
also kind of a super nit, but with C++ iterators you generally want to pre-increment, as post increment will make a copy and that copy constructor might not be optimized away by the compiler.
Resources: http://www.gotw.ca/gotw/002.htm, https://stackoverflow.com/questions/1077026/incrementing-iterators-is-it-more-efficient-than-it
| raw_params.reserve(params.size()); | ||
| for (const auto ¶m : params) { | ||
| raw_params.push_back(param.val); | ||
| raw_params.reserve(end - begin); |
There was a problem hiding this comment.
You might want to static assert or conditionally do this only if it's a random access iterator.
There was a problem hiding this comment.
Oh good point, what's the best way to do that in C++?
There was a problem hiding this comment.
If you only support C++20 or greater than the easiest way is:
if constexpr (std::random_access_iterator<I>) {
raw_params.reserve(end - begin);
}In C++ 17 you'll have to use SFINAE as I understand it.
2 participants
Also update some utilities on
Valwhich can no longer be implemented without a store parameter.