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

[Nevin Liber via Phabricator via llvm-commits]

nliber added a comment.

[Sorry it took so long to get back to this...]

- Consider putting `swap` into a separate "operator" class (it isn't really an operator, but it is close...), which would be inherited like everything else.
- Move construction, move assignment and `swap` are the three which really need `noexcept` on them, and you already get it for free with move construction/assignment because you are applying the "Rule of Zero".  As was pointed out, `is_nothrow_swappable` didn't show up until C++17.  However, I do not believe its implementation relies on anything in C++17.  Consider implementing the trait yourself and using it (or equivalent) on `swap`.
- Instead of default construction," copy the value" construction and "moving the value" construction, consider having a templated constructor which does emplace construction into `Value`.  Not only does this cover the three construction methods already mentioned, it can be more efficient if you do not have the value constructed already, and it also enables wrapping of non-copyable non-moveable types (such as `atomic<T>`).  It would look something like (caution: untested):

  template<typename... Us, typename = std::enable_if_t<std::is_constructible<T, Us...>::value>>
  constexpr SemanticTypedef(Us&&... us)
  noexcept(noexcept(T(std::forward<Us>(us)...)))
  : Value(std::forward<Us>(us)...)
  {}

Note:  even if you don't want to constrain it to `is_constructible`, you still need to constrain it so that non-`const` instances of `SemanticTypedef` bind to the copy constructor instead of this templated constructor.  IIRC this doesn't cover `std::initializer_list` constructors, so you would need to make forward constructors if you wished to support forwarding those.

- Consider adding a named getter member function (such as `.get()` or `.ref()`) that returns a (`const`) reference as well as the conversion operator.  It is useful to have a short name when you need access to the underlying value.
- Consider adding a `typedef` (such as `value_type`) inside `SemanticTypedef` for the underlying type (and I would have a `tag_type` for good measure).  You know what it is, and users shouldn't have to go through hoops to get it.  Heck, `UnderlyingType` and `TagType` shouldn't have to go through hoops to get it.
- Consider eliminating `Sortable`.  Types which only define `operator<` are weird, and we shouldn't be encouraging weird types.
- Consider adding heterogeneous operations with `SemanticTypedef<Tag, T>` and `T` (in either order).  For instance, one shouldn't have to construct a `SemanticTypedef<Tag, string>` if they just want to compare a `string` with a `SemanticTypedef<Tag, string>`.
- Consider defining all operators except assignment as friends.  Anthony Williams's blog post on hidden friends <https://www.justsoftwaresolutions.co.uk/cplusplus/hidden-friends.html> explains why.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66148