[PATCH] D66148: [SemanticTypedef] Provide a semantic typedef class and operators

[David Greene via Phabricator via llvm-commits]

greened marked an inline comment as done.
greened added inline comments.


================
Comment at: llvm/include/llvm/ADT/SemanticTypedef.h:111
+  constexpr SemanticTypedef() : Value() {}
+  constexpr SemanticTypedef(const SemanticTypedef<Tag, T> &) = default;
+  explicit constexpr SemanticTypedef(const T &V) : Value(V) {}
----------------
greened wrote:
> greened wrote:
> > nliber wrote:
> > > This line gives you copy construction.  Because there are no other declarations for copy/move constructor/assignment operator or destructor, you also get copy assignment but move construction/assignment is effectively deleted.  Also deriving from Assign<Typedef> does not change this.
> > > 
> > > Recommendation:  remove this line of code.
> > Deriving from `Assign<Typedef>` works.  It's in the unit tests.  Perhaps I've missed something.
> > 
> > I can add a move constructor.  That was an oversight, thanks for pointing it out!
> Ok, I think I see.  I misunderstood what you were getting at.  Deriving from `Assign` doesn't provide move construction.  It does provide move assignment though.  Wouldn't adding a move constructor fix the issue?
After thinking about this some more, I think you are right.  I will delete the default copy constructor and also the `Assign` operator since it's hidden by the default assignment operator anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66148