Move execute(initializer_list) to fizzy::test #657
Conversation
chfast
force-pushed
the
test_execution_helpers
branch
from
a1b2d4f
to
a04d8a2
Compare
| @@ -38,11 +38,4 @@ constexpr ExecutionResult Trap{false}; | |||
|
|
|||
| // Execute a function on an instance. | |||
| ExecutionResult execute(Instance& instance, FuncIdx func_idx, const Value* args, int depth = 0); | |||
|
|
|||
| inline ExecutionResult execute( | |||
| Instance& instance, FuncIdx func_idx, std::initializer_list<Value> args) | |||
There was a problem hiding this comment.
- The
std::initializer_list<Value>cannot be used in public API like that. This is not what it is designed for. We can usestd::spaninstead. - I prefer to change it to
std::initializer_list<TypedValue>later instead of having a ton of overloads spread accross many places.
There was a problem hiding this comment.
The std::initializer_list cannot be used in public API like that. This is not what it is designed for. We can use std::span instead.
Not arguing against, but can you explain the reason for this?
There was a problem hiding this comment.
This usage is probably fine, but there are some doubts about the lifetime of the {} object. The std::initializer_list is to provide fancy syntax to containers. Somehow in C++20 had been decided that std::span cannot reference std::initializer_list.
In practice this is mostly useful in cases when you want to provide some fixed number of arguments, i.e. mostly tests and examples. None of the other tools need it.
Codecov Report
@@ Coverage Diff @@
## master #657 +/- ##
=======================================
Coverage 98.38% 98.38%
=======================================
Files 69 69
Lines 9724 9724
=======================================
Hits 9567 9567
Misses 157 157
Flags with carried forward coverage won't be shown. Click here to find out more.
|
chfast
force-pushed
the
test_execution_helpers
branch
from
a04d8a2
to
892b3ba
Compare
No description provided.