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

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D96817#2569852 <https://reviews.llvm.org/D96817#2569852>, @clayborg wrote:

> In D96817#2569595 <https://reviews.llvm.org/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 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96817/new/

https://reviews.llvm.org/D96817