Reimplementing constraint solver

[abdullah]

Maybe the reinterpret_cast can be encapsulated with appropriate type checking like this. Still not the best solution. Probably better than before.

Code: Select all

void FCS::G2D::ParaConic::initAttrs()
{
    ParaCurve::initAttrs();

    tieAttr_Child(p0.upcast<ParaObject>(), "p0", &ParaPointPy::Type, true);
    tieAttr_Child(reinterpret_cast<HParaObject &>(p1), "p1", &ParaPointPy::Type, true);
}

N.B.: The previous code shows one with the upcast method and the other with the inplace reinterpret_cast

with

Code: Select all

   template <  typename NewTypeT,
                typename = typename std::enable_if<
                    std::is_base_of<typename std::decay<NewTypeT>::type, CppType>::value
             >::type
    >
    UnsafePyHandle<NewTypeT> & upcast() 
    {
        return reinterpret_cast<UnsafePyHandle<NewTypeT> &>(*this);
    }

[DeepSOIC]

In this context using a PyHandleBase in ConstraintSolver looks ilogical and counterintuitive. It is not a solution, but a work-around to this specific issue.

I'm sorry, but I disagree. There is an issue, PyHandle<A> and PyHandle<B> are unrelated. It gets in the way. reinterpret_cast is a workaround: the issue is still there, we're just telling the compiler "shut up and continue". Adding a base class actually solves the issue: the types become related, so it's a solution, not a workaround.

Avoiding the need to make the types related is another thing I'd call a "solution", but I don't see how to do it conveniently yet.

[DeepSOIC]

I see you are scared of the little reinterpret_cast

yes I am. Casts are weapons; reinterpret_cast is the most lethal of them all. I am yet to be convinced this cast is not an undefined behavior. Because I've read that memory layout of classes is compiler's implementation detail, and I think there isn't a requirement for that to even be consistent (think optimization, although I don't think much is going to happen to PyHandle).

[DeepSOIC]

There is no better documentation than code that speaks for itself. In a far fetched scenario, that beauty is protecting you in case the implementation of the function changes (during maintenance... by somebody like me...) and the compilation error is gone.

True.

[DeepSOIC]

Another totally different scenario would be that you would indeed use both UnsafePyHandles and SafePyHandles in ConstraintSolver and these needed to be treated seamlessly in some scenario.

I can't really say it can be ruled out. In fact, at one point I thought of replacing all HParaSomething data members of geometries and constraints with safe handles, and remove type argument for tieAttrs, since the handle will take care of type checking. But then, I realized it may be tricky to emit an informative error message, + I get a performance hit (probably negligible; experimentation required). So things remained.

[DeepSOIC]

I also would like to admit that I'm guilty of a similar reinterpret-cast-like situation in constraintsolver.

ParaObject::setAttr for attributes tied with tieAttr_Shape allows a HParaGeometry of corresponding type as input. Because the subclass of ParaShape corresponding to the geometry type is not known upfront, a ParaShapeBase is constructed to wrap that HParaGeometry, and assigned to that data member of (say) constraint through the reference given to tieAttr.
To simplify, something like this is done:

Code: Select all

aconstraint.point1 = static_cast<ParaShape<Point>*>(new ParaShapeBase(geom))

that is, point1 attribute now points to an object that was never constructed as ParaShape<ParaPoint>.

Again, they share memory footprint, because specialized versions of ParaShape have no extra data members and no overridden functions compared to ParaShapeBase. However, if we were to dynamic_cast the object pointed by point1 attribute, we would get a surprising result (i.e., it will refuse to dynamic_cast to ParaShape<Point>).

[DeepSOIC]

Merged.

I have also given you write access to the repo (hopefully), so that you can commit stuff without asking. Of course, I expect discussions in case it is something substantial.

[triplus]

Good to hear an agreement was reached on this level.

[abdullah]

I'm sorry, but I disagree. There is an issue, PyHandle<A> and PyHandle<B> are unrelated. It gets in the way. reinterpret_cast is a workaround: the issue is still there, we're just telling the compiler "shut up and continue". Adding a base class actually solves the issue: the types become related, so it's a solution, not a workaround.

Avoiding the need to make the types related is another thing I'd call a "solution", but I don't see how to do it conveniently yet.

I am not convinced, but I agree that it would be great not to need this at all. I will put it in the todo list.

In the meantime, I have encapsulated the reinterpret_cast so as not to require it the client code and provide type checking that ensures that A and B are related by inheritance. I have PR-ed it, as github rejects my push.

yes I am. Casts are weapons; reinterpret_cast is the most lethal of them all. I am yet to be convinced this cast is not an undefined behavior. Because I've read that memory layout of classes is compiler's implementation detail, and I think there isn't a requirement for that to even be consistent (think optimization, although I don't think much is going to happen to PyHandle).

Even if memory layout of classes were compiler implementation specific, a same compiler should consistently produce a same memory layout for PyHandle's. The extra test now goes beyond that and enforces at compilation time that A and B are related and it is an upcast.

ParaObject::setAttr for attributes tied with tieAttr_Shape allows a HParaGeometry of corresponding type as input. Because the subclass of ParaShape corresponding to the geometry type is not known upfront, a ParaShapeBase is constructed to wrap that HParaGeometry, and assigned to that data member of (say) constraint through the reference given to tieAttr.

I thought it odd to see ParaShapeBase in the code, but I have not yet studied what is happening under hood there. I put a note to self.

Merged.

I have also given you write access to the repo (hopefully), so that you can commit stuff without asking. Of course, I expect discussions in case it is something substantial.

I celebrate your constructive spirit . I have tried to commit stuff without asking, but Github rejected ... ahh! wise GitHub!!

[DeepSOIC]

I have tried to commit stuff without asking, but Github rejected ... ahh! wise GitHub!!

I don't know, github says the invite is still pending. I'll look for any other places, maybe I just tried the wrong one.