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

[Nevin Liber via Phabricator via llvm-commits]

nliber added a comment.

A strong motivation for using this over `enum class` should be provided.  The default behavior is too minimalistic.

In my experience, the reason people use `bool`, `int`, `string`, etc. is because they provide a rich set of operations out of the box:  assignment, comparison, implicit conversions, etc.  `enum class` also provides most of those operations (other than implicit conversion) and has two benefits: it introduces a new type, and it makes it easy to name constants.  `SemanticTypedef`, on the other hand, provides almost no operations without a lot more declaration.

If you use derivation (as opposed to using/typedef), you don't even get the converting constructor without an extra declaration.  While derivation does have a benefit (you don't have to invent a tag type), it has a fairly high cost.  Also, public derivation is used (not strictly necessary for introducing friend functions but necessary for everything else) even though this does not model an is-a relationship.  The idea for using CRTP for this has been here for over two decades (e.g. Boost.Operators is one of the oldest Boost libraries) and yet, for the most part, hasn't caught on.

It is rare to have a value type that does not have assignment or equality comparisons (and hashing, as that goes along with equality comparisons).  The default should minimally provide this behavior.



================
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) {}
----------------
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.


================
Comment at: llvm/include/llvm/ADT/SemanticTypedef.h:125
+
+  friend void swap(SemanticTypedef &A, SemanticTypedef &B) {
+    using std::swap;
----------------
This needs an exception specification.


================
Comment at: llvm/include/llvm/ADT/SemanticTypedef.h:183
+/// Define a macro to create unary operators.
+#define SEMANTIC_TYPEDEF_UNARY_OP(Op, ResultType)               \
+  friend ResultType operator Op(const Typedef &O) {             \
----------------
The downside to using macros here is that it makes it harder for users to debug code going through these macros.  It doesn't seem worth writing eight macros just to write six classes.


================
Comment at: llvm/include/llvm/ADT/SemanticTypedef.h:206
+    return ResultType(                                          \
+        detail::getValue(static_cast<const Typedef &>(LHS)) Op  \
+        detail::getValue(static_cast<const Typedef &>(RHS)));   \
----------------
Why the casts?


================
Comment at: llvm/include/llvm/ADT/SemanticTypedef.h:238
+/// Define a macro to declare comparison operators and the base class.
+#define SEMANTIC_TYPEDEF_MAKE_COMPARE_OP(Name, Op, Alt)                 \
+  /* Operator(Typedef, Typedef) */                                      \
----------------
This is not true in general.  The canonical example is floating point NaNs.  See std;:optional for how comparisons when wrapping a single value should work when comparing the underlying value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66148