Skip to content

Conversation

@ejpbruel2
Copy link
Contributor

@ejpbruel2 ejpbruel2 commented Dec 7, 2017

Our ultimate goal here is to build an interpreter for a stack machine. The primary data structure in this stack machine is of course the stack. Our stack should be able to contain values of arbitrary types. To accomplish this, we now introduce the Value type, which can take on several different types. This Value type has been carefully designed to perform well on a stack (see the comments for details).

Finally, the is<T> and get<T> helper functions will allow us to use the Value class in a generic context. This will prove useful when we use templates to automatically wrap built-in functions.

Value::Value(const Vector_2& v)
: type_(Type::vector_2),
vector_2_(std::unique_ptr<const Vector_2>(new Vector_2(v)))
{

Choose a reason for hiding this comment

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

using new in member initialization is imo not a good idea. if construction fails because of memory allocation error and many members were initialized in this way, it is impossible to find which member caused the problem, and the resources for the members are leaked. furthermore, it makes it unclear which members need to be freed. However, since the new is within the construction of the unique_ptr the latter argument may not be valid, i'm not sure on this.


Value&
Value::operator=(Value&& x)
{

Choose a reason for hiding this comment

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

doesn't a referenced reference implicitly get cast to a reference making this assignment operator redundant ?

case Type::vector_2:
new (&vector_2_)
std::unique_ptr<const Vector_2>(new Vector_2(*x.vector_2_));
break;

Choose a reason for hiding this comment

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

just for my information, why do you need the second 'new' here? couldnt you just construct using the x.vector_2_ directly ?

@jochemborges
Copy link

maybe it wasn't clear in our discussion on the math types, but I really urge you to remove the vector2 and matrix3 classes altogether. we will not use them often, they are only intended to reduce datatransfer but to a very small extend, and it increases the maintenance burden that is already considerable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants