[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

tatyana-krasnukha created this revision. tatyana-krasnukha added reviewers: clayborg, JDevlieghere, labath, teemperor. tatyana-krasnukha added a project: LLDB. Herald added a subscriber: mgorny. tatyana-krasnukha requested review of this revision. Herald added a subscriber: lldb-commits. Some impl

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

dblaikie added a comment. Perhaps all these clone implementations could be provided via mixin instead? struct base { virtual std::shared_ptr Clone() = 0; ... }; template struct base_clone_helper : IntermediateBase { static_assert(std::is_base_of::value); std::shared_ptr Cl

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

dblaikie added inline comments. Comment at: lldb/unittests/Interpreter/TestOptionValue.cpp:173 + // Trigger the callback second time. + file_list_copy_ptr->SetValueFromString(llvm::StringRef("0 another/path"), + eVarSetOperationReplace);

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

tatyana-krasnukha added a comment. 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 i

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

teemperor added inline comments. Comment at: lldb/unittests/Interpreter/TestOptionValue.cpp:173 + // Trigger the callback second time. + file_list_copy_ptr->SetValueFromString(llvm::StringRef("0 another/path"), + eVarSetOperationReplace);

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

dblaikie added a comment. > 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

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

tatyana-krasnukha updated this revision to Diff 324435. tatyana-krasnukha added a comment. Removed explicit conversions to StringRef from the test with respect to D96861 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96817/new/ https://reviews.llvm.org

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

clayborg added a comment. In D96817#2569595 , @dblaikie wrote: >> 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 overridde

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

dblaikie added a comment. In D96817#2569852 , @clayborg wrote: > In D96817#2569595 , @dblaikie wrote: > >>> CRTP was my first implementation, however, I discarded it as more >>> bug-prone. Virtual Clone function at

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

tatyana-krasnukha added a comment. I created D96817 - a demonstrative example of the CRTP-based version of this patch. Please, take a look. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96817/new/ https://reviews.llvm.org/D96817 __

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

tatyana-krasnukha abandoned this revision. tatyana-krasnukha added a comment. D96952 is landed instead. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96817/new/ https://reviews.llvm.org/D96817 ___ lldb-commit