Replace unsafe C functions Proposal

Development
9

So after some discussion of how this can go, some things that came up:

Compatibility

So vtkStdString is on my list of things to go, but we can use its drawback to help bridge the compatibility here. It has an implicit const char* conversion operator (very dangerous, but no worse than C pointers anyways). We can use this to our advantage here. Note that classes that use nullptr as some magic value would need something like vtkOptionalString with convenience methods to convert to nullptr if it is none. Converting a char* member to vtkStdString can be kept compatible with:

Old code:

vtkSetStringMacro(Member);
// virtual void SetMember(const char* _arg) { … }
vtkGetStringMacro(Member);
// virtual char* GetMember() { … }

char* Member;

New code:

nullptr is invalid:

void SetMember(const char* _arg) { // Need to handle `nullptr` callers
    if (_arg) {
        this->SetMember(vtkStdString(_arg));
    }
}
void SetMember(vtkStdString _arg); // impl in C++ code (see below)
vtkStdString const& GetMember() const { return this->Member; }

vtkStdString Member;

// --------------

void SetMember(vtkStdString _arg) {
    if (_arg != this->Member) {
        this->Member = std::move(_arg);
        this->Modified();
    }
}

nullptr is valid:

void SetMember(vtkOptionalString _arg); // impl in C++ code (same body as above)
vtkOptionalString const& GetMember() const { return this->Member; }

vtkOptionalString would be another interim class to help bridge the gap here which just turns nullptr into std::none_t. It’s implicit const char* conversion would similarly be deprecated someday (just like vtkStdString's).

vtkSetGet macros

I find these macros to be more noise than useful. About the only thing they do is:

  • avoid typos
  • ensure Modified() is called properly in setters

Other than that, I see no benefit. I think that unit tests are far better for ensuring such things. The benefits:

  • virtual would no longer be used for these things
  • getters could be const
  • getters could return const& instead of forcing a copy
  • setters could std::move for non-POD types (allowing the caller to decide if memory is reused instead of pessimizing and forcing a copy even when the caller doesn’t need the string anymore)
  • headers lose a lot of unnecessary code that triggers a full rebuild when vtkSetGet is changed
  • the member name is no longer pinned to the method name (allowing for more PIMPL usage)

The inline-ness is largely useless for the setters because VTK doesn’t support devirtualization of Modified calls (the use of factory constructors hides necessary knowledge even for vtkNew<vtkObject> obj; obj->SetSomething();), so the code is the same for every single caller and any actual inlining is of no benefit (it just inflates the caller’s code size).