feat: evaluation algorithm #166
Conversation
…legates to std::optional<T> overloads
Lacking rule matching logic, which is stubbed out. Also lacking store interface.
| * false.) | ||
| */ | ||
| explicit operator bool() const; | ||
|
|
There was a problem hiding this comment.
Unclear to me if we need both IsError and operator bool().
The intention is to allow users (and us, in test code) to check if the evaluation has an error, without having to reach deep into the object.
Having if (detail) is pretty convenient and fits in with std::optional and tl::expected, I just don't know if we need the explicit IsError as well.
There was a problem hiding this comment.
std::optional, and tl::expected both have kind of pointer like behavior, and they both additionally have an explicit way to check for a value has_value. We don't really have pointer like behavior, so I don't know that the boolean operator would be very apparent. It isn't bad to have, but an error and having a value are a bit different for an evaluation reason.
There was a problem hiding this comment.
It's true it's not pointer-like, but we do have operator* to get at the value. So you could do:
if (result) {
double x = (*result).AsDouble()
}But this does feel clunky. Hmm.
There was a problem hiding this comment.
Perhaps the VariationDetail methods should really accept detail as an out-param, or just not have VariationDetail at all and use overloads instead.
There was a problem hiding this comment.
Producing an evaluation result seems totally reasonable, the VariationDetail methods are typed, so there should be no AsDouble. Unless you really just want to make yourself more work.
If you just are using it for a side effect, then you could deref it. I would likely just call .Value() myself. It isn't conditional, it is just a value.
An out variable, in my view, just makes it even more inconvenient when you may not care about it at all.
There was a problem hiding this comment.
Generally speaking I don't think you usually even care if there was an error.
cwaldren-ld
force-pushed
the
cw/sc-206689/evaluator
branch
from
ab3a130 to
3c4bbf9
Compare
| if (!segment.salt) { | ||
| return tl::make_unexpected(Error::MissingSalt(segment.key)); | ||
| } | ||
| if (Match(rule, context, store, stack, segment.key, *segment.salt)) { |
There was a problem hiding this comment.
Is this correct? Reflexively it feels incorrect, but maybe I am not following the logic correctly (I would expect the value to be used, versus presence of value?).
There was a problem hiding this comment.
Yes, this is wrong. I missed that this was returning a tl::expected. Updated and added a unit test to make sure this piece is getting exercised.
kinyoklion
left a comment
kinyoklion
left a comment
There was a problem hiding this comment.
Inline question about segment matching.
This PR contains the main evaluation engine and associated tests.