Incomplete Objects Passed To Base Classes

[weshoke]

I noticed this pattern in Polyscope that's problematic from a memory point of view. Passing *this to a base class is passing an object that's not completely formed and can have memory access issues. For example, imagine VolumeMeshVertexVectorQuantity had a pointer member. That value would only be initialized after all base classes are initialized. If that value is needed by some computation within a base class like a call to uniquePrefix, there will be a segfault.

VolumeMeshVertexVectorQuantity::VolumeMeshVertexVectorQuantity(std::string name, std::vector<glm::vec3> vectors_,
                                                               VolumeMesh& mesh_, VectorType vectorType_)

    : VolumeMeshVectorQuantity(name, mesh_, VolumeMeshElement::VERTEX),
      VectorQuantity<VolumeMeshVertexVectorQuantity>(*this, vectors_, parent.vertexPositions, vectorType_) {}

[nmwsharp]

There are a lot of poorly-chosen patterns in the internal polyscope code :) It was one of my first codebases after learning C++ and you can tell at times! I just fixed another round of obnoxious static destructor bugs caused by similar poor decisions.

Anyway, I agree that it would definitely be better to avoid this pattern. But with that being said, I believe it is technically safe & compliant as long as we do not actually access any of the uninitialized members in constructors. For instance in the VectorQuantity<> in this example, the constructor body is empty. Does that sound right? Have you noticed any cases where there is actually an illegal access?

I mainly ask because rewriting the code to avoid these patterns is potentially a lot of work! If there are any invalid accesses, we might want to spot-fix them instead.

[weshoke]

You're right that as long as you don't access any uninitialized members, you're ok. That said, I was motivated to post this while doing some work to add a ScalarQuantity to a VectorQuantity so that I could color the vectors based on other data. Here, I was trying to use some data from the parent quantity to build the uniqueName, but it was uninitialized. I then realized that all of the unique names are using static data, so it's ok.

The multiply recursive nature of these quantities is complicated. I understand why it was done this way, but it made even getting a basic version of what I was trying to do very difficult. Maybe there's another way? In the end, I hacked it in in a way that's really not useful for anyone but me. I just don't see a way forward without a large scale change.

[nmwsharp]

Ah yep, that makes perfect sense.

A lot of these patterns were contortions to support something which was totally not-worth-it in retrospect. Once upon a time I thought it was really important to support chaining in setters, like quantity.setEnabled(true).setColormap("viridis").setRadius(12.);. I have no idea why I thought this was so important. But that only works if setEnabled() returns the child class, rather than a base class. So I went crazy with templates etc to get some inheritance but still return the child class. I think the proper fix is to drop support for this paradigm, and move much of the code back to a simpler class hierarchies with fewer templates and silly references.

But sadly that would be a breaking change AND a lot of work, so it probably will never happen :) I don't know of any clever tweaks to improve the situation without much work. But I'm open to ideas!

[nmwsharp]

I'll close this for now since I'm not sure there's an action-item. However feel free to reopen if there are more issues or proposed fixes!