Switch Value to use the inherit/cast model #703
Conversation
There was a problem hiding this comment.
Minor note, I do
cast<Type>(*val).Call()for consistency with the Pattern change, but I thinkcast<Type>(val)->Call()would work the same. I wanted to note this if it wasn't a deliberate choice, before we dig in on this route.
I'd sort of prefer to avoid cast<Type>(val) when val is a pointer because it feels a little too DWIM-ish the cast says Type but the result is actually Type*. I'd be fine with cast<Type*>(val), though, assuming that works.
| CHECK(false) | ||
| << "arity mismatch in tuple pattern match:\n pattern: " | ||
| << p_tup << "\n value: " << v_tup; | ||
| FATAL_RUNTIME_ERROR(line_num) | ||
| << "arity mismatch in tuple pattern match"; | ||
| << "arity mismatch in tuple pattern match:\n pattern: " | ||
| << p_tup << "\n value: " << v_tup; |
There was a problem hiding this comment.
I'm guessing you meant to delete one of these?
There was a problem hiding this comment.
Yes, sorry about that, the CHECK was for debugging
| @@ -759,7 +762,8 @@ void StepPattern() { | |||
| if (act->pos == 0) { | |||
| if (tuple.Fields().empty()) { | |||
| frame->todo.Pop(1); | |||
| frame->todo.Push(Action::MakeValAction(Value::MakeTupleValue({}))); | |||
| frame->todo.Push(Action::MakeValAction( | |||
| global_arena->New<TupleValue>(std::vector<TupleElement>()))); | |||
There was a problem hiding this comment.
Maybe we should give TupleValue a default constructor that's equivalent to constructing from an empty vector? It's a little inelegant, but I expect creating empty tuples (e.g. the unit type) to be a moderately common pattern, so it'd be nice to make it less verbose.
(It's so frustrating that C++ doesn't give us better options here)
There was a problem hiding this comment.
I'll send a separate PR for this, I think it'll be easier to review that way.
| auto Name() const -> const std::optional<std::string>& { return name; } | ||
| auto Type() const -> const Value* { return type; } | ||
|
|
||
| private: | ||
| // nullopt represents the `_` placeholder |
There was a problem hiding this comment.
This comment should probably go on the accessor
Minor note, I do
cast<Type>(*val).Call()for consistency with the Pattern change, but I thinkcast<Type>(val)->Call()would work the same. I wanted to note this if it wasn't a deliberate choice, before we dig in on this route.