This is an archive of the discontinued LLVM Phabricator instance.

Fix deep copying for OptionValue classes
AbandonedPublic

Authored by tatyana-krasnukha on Feb 16 2021, 2:15 PM.

Details

Summary

Some implementations of the DeepCopy function called the copy constructor that copied m_parent member instead of setting a new parent. Others just lived the base class's members (m_parent, m_callback, m_was_set) empty.
One more problem is that not all classes override this function, e.g. OptionValueArgs::DeepCopy produces OptionValueArray instance, and Target[Process/Thread]ValueProperty::DeepCopy produces OptionValueProperty. This makes downcasting via static_cast invalid.

This patch adds a virtual Clone function, which implements well-known idiom "virtual constructor", and overrides it in every derived class. DeepCopy calls Clone to instantiate an object of a correct type. It also sets valid m_parent.
Add a test that checks DeepCopy for correct copying/setting all data members of the base class.

Diff Detail

Event Timeline

tatyana-krasnukha requested review of this revision.Feb 16 2021, 2:15 PM

Perhaps all these clone implementations could be provided via mixin instead?

struct base {
  virtual std::shared_ptr<base> Clone() = 0;
  ...
};
template<typename Derived, typename IntermediateBase = base>
struct base_clone_helper : IntermediateBase {
  static_assert(std::is_base_of<base, IntermediateBase>::value);
  std::shared_ptr<base> Clone() const override {
    return std::make_shared<Derived>(static_cast<const Derived&>(*this*));
  }
};
struct some_immediate_derivation final : base_clone_helper<some_immediate_derivation> {
};
struct some_intermediate_helper : base {
  ...
};
struct some_concrete_derivation final : base_clone_helper<some_concrete_derivation, some_intermediate_helper> {
};

Requiring that all such types be copy constructible (I guess the template helper could also provide an rvalue overload of Clone if there's a need to have move cloning, though that seems unlikely to be needed/useful)

lldb/unittests/Interpreter/TestOptionValue.cpp
173

Generally it shouldn't be necessary to write llvm::StringRef(...) around a string literal - StringRef is implicitly convertible from a string literal.

CRTP was my first implementation, however, I discarded it as more bug-prone. Virtual Clone function at the interface is so common that, I believe, everyone knows it must be overridden by a new derived class. The necessity of inheriting from base_clone_helper is not so obvious.

lldb/unittests/Interpreter/TestOptionValue.cpp
173

OptionValueProperties and OptionValueFileSpecList enforce using StringRef explicitly by specifying the overload SetValueFromString(const char*) as deleted.

teemperor added inline comments.
lldb/unittests/Interpreter/TestOptionValue.cpp
173

So I was curious why we added these deleted overloads and from what I can see this was done as part of some C-string -> StringRef conversion in D24847. I don't think these overloads serve any purpose anymore, so I made a review for removing them: D96861

CRTP was my first implementation, however, I discarded it as more bug-prone. Virtual Clone function at the interface is so common that, I believe, everyone knows it must be overridden by a new derived class. The necessity of inheriting from base_clone_helper is not so obvious.

I would've thought it'd be pretty easy to accidentally miss either of these - I think the CRTP helper ensures consistency of implementation (harder to accidentally slice/copy the wrong type/etc. But I'm not a code owner/major contributor to lldb specifically, so probably more up to other developers who are.

Removed explicit conversions to StringRef from the test with respect to D96861.

CRTP was my first implementation, however, I discarded it as more bug-prone. Virtual Clone function at the interface is so common that, I believe, everyone knows it must be overridden by a new derived class. The necessity of inheriting from base_clone_helper is not so obvious.

I would've thought it'd be pretty easy to accidentally miss either of these - I think the CRTP helper ensures consistency of implementation (harder to accidentally slice/copy the wrong type/etc. But I'm not a code owner/major contributor to lldb specifically, so probably more up to other developers who are.

Whatever is the safest and most llvm like is probably the best approach IMHO. So maybe stick with a solution that will be familiar to LLVM developers if this current approach isn't. I am open to other suggestions if others feel strongly otherwise.

CRTP was my first implementation, however, I discarded it as more bug-prone. Virtual Clone function at the interface is so common that, I believe, everyone knows it must be overridden by a new derived class. The necessity of inheriting from base_clone_helper is not so obvious.

I would've thought it'd be pretty easy to accidentally miss either of these - I think the CRTP helper ensures consistency of implementation (harder to accidentally slice/copy the wrong type/etc. But I'm not a code owner/major contributor to lldb specifically, so probably more up to other developers who are.

Whatever is the safest and most llvm like is probably the best approach IMHO. So maybe stick with a solution that will be familiar to LLVM developers if this current approach isn't. I am open to other suggestions if others feel strongly otherwise.

I think either would be legible/familiar to LLVM developers. I rather like the CRTP, but it's not a make-or-break.

I found a couple of instances of cloning APIs across LLVM (incomplete results - grep only gets one so far):

clang/include/clang/Sema/TypoCorrection.h:  virtual std::unique_ptr<CorrectionCandidateCallback> clone() = 0;
lldb/include/lldb/Core/SearchFilter.h:  virtual lldb::SearchFilterSP DoCreateCopy() = 0;

And neither uses a CRTP for the derive classes, FWIW.

tatyana-krasnukha added a comment.EditedFeb 18 2021, 7:51 AM

I created D96952 - a demonstrative example of the CRTP-based version of this patch. Please, take a look.

tatyana-krasnukha abandoned this revision.Feb 28 2021, 8:29 AM

D96952 is landed instead.