Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapt nyan to new specification #84

Closed
wants to merge 71 commits into from
Closed

Conversation

heinezen
Copy link
Member

@heinezen heinezen commented Oct 5, 2020

Introduces the features discussed for version 1 of the nyan spec.

@heinezen heinezen changed the title Adapt nyan to new specification [WIP] Adapt nyan to new specification Oct 6, 2020
@heinezen heinezen changed the title [WIP] Adapt nyan to new specification Adapt nyan to new specification Dec 23, 2020
Copy link
Member

@TheJJ TheJJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice!
the only (major) thing i'm not happy about is the none-value assignment validation.
we should do it properly, even if it means some restructuring.

the validation if none can be assigned should be done by recursively checking from the inside of a type until we reach the type root, and there we just validate the allowed ops of this composite type with the given value.
if there's no ops, then we step up one step of the composite type and validate if there's any allowed ops in the next type. if there's no allowed ops when we reach the type root, the assignment is not allowed.

throw Error{"dividing two inf values not permitted"};

default:
throw Error{"unknown operation requested"};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these should be "runtime errors"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InternalError or a new error class?

return 0;

default:
throw Error{"unknown operation requested"};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these should not be Error, but something else that indicates this is a user-value-induced-problem, and it's a runtime error, and maybe that it's specifically invalid infinity handling

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could apply the same restrctions to inf as we did for None: None can only be assigned (=). But even then we counter encounter the inf * 0 case at runtime (assuming that x / 0 is handled for all numbers).

Float *left = dynamic_cast<Float *>(this);
auto change_value = left->handle_infinity(change, operation);
Float left = static_cast<Float>(*this);
auto change_value = left.handle_infinity(change, operation);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't it work with pointers? or is this just an optimization?

Copy link
Member Author

@heinezen heinezen Jan 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I do

Float *left = static_cast<Float *>(this);

it complains about

error: invalid ‘static_cast’ from type ‘nyan::Number<long int>*’ to type ‘nyan::Float*’ {aka ‘nyan::Number<double>*’}
[build]   147 |             Float *left = static_cast<Float *>(this);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't see that the typecheck above is about operand, not about this.

So the error makes sense since *this can be Float, Int or any other template instance of Number. If we statically cast this to Float* in case this is actually Int*, it would mean we do an invalid side-cast. When we don't have ptrs/refs (like you have now), it actually calls the implicit conversion from *this to Float (i.e. Number(T value) and the to-float/to-int conversion from operator T()).

So we do get a new Float object that is created from the int-object, since it basically calls this: Float left{this->get()}; But this doesn't handle the cases when the values are infinite. We would need explicit conversions from Float to Int and vice versa, where we take over the inf values.

But what happens when change_value (which will be of Float here) is inf, and we now apply it to this->value? It's also not handled, we need to back-convert the float-inf to a int-inf. So i suspect we need a better general approach instead of converting back and forward.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(done in #87 )

nyan/value/dict.cpp Outdated Show resolved Hide resolved
builder << util::strjoin(
", ", this->values,
[] (const auto &val) {
return val.first->str() + ": " + val.second->str();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the python standard is to always use repr for the dict members. so Dict::str and Dict::repr can be identicall, and use the repr variant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so just use return this->repr(); in this function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, i'd say so.

if (with_type.get_primitive_type() == primitive_t::OBJECT) {
switch (with_type.get_primitive_type()) {
case primitive_t::OBJECT:
case primitive_t::NONE:
return ops;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can we assign = None to an object?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -88,6 +88,13 @@ const std::unordered_set<nyan_op> &OrderedSet::allowed_operations(const Type &wi
nyan_op::INTERSECT_ASSIGN,
};

if (with_type.get_primitive_type() == primitive_t::NONE) {
return none_ops;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can we assign = None to an orderedset?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -90,6 +90,14 @@ const std::unordered_set<nyan_op> &Set::allowed_operations(const Type &with_type
nyan_op::INTERSECT_ASSIGN,
};

if (with_type.get_primitive_type() == primitive_t::NONE) {
return none_ops;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can we assign = None to a set?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else {

case primitive_t::NONE:
return none_ops;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can we assign = None to a text?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


if (modifier_type == composite_t::OPTIONAL) {
contains_optional = true;
}
Copy link
Member

@TheJJ TheJJ Jan 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting corner case, which optional do we assign to in optional(optional(int)) = None (it won't matter in practice since the returnvalue is None in either way). or optional(bool) = None where it will matter in practice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is just assigned to the member because optional does not have its own Value instance. In the current implementation optional(optional(int)) is treated the same as optional(int).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok that resolves that, but what about optional(bool) = None? we have do decide on a behavior there, i'd say we try to assign it from the inside to the outside until the operator = is accepted. then we also don't need all the none-assignments in all the types

@heinezen heinezen requested a review from TheJJ January 15, 2021 12:17
@TheJJ TheJJ self-assigned this Jan 21, 2021
@TheJJ TheJJ mentioned this pull request Feb 16, 2021
9 tasks
@TheJJ
Copy link
Member

TheJJ commented Feb 16, 2021

I'll continue this in #87

@TheJJ TheJJ closed this Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants