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

[David Greene via Phabricator via llvm-commits]

greened marked 6 inline comments as done.
greened added inline comments.


================
Comment at: llvm/include/llvm/ADT/SemanticTypedef.h:1
+//===-- SemanticTypedef.h - Typedefs for a type-safe world ------*- C++ -*-===//
+//
----------------
labath wrote:
> I know this has been discussed on the thread, but I feel obligated to point out that there is already precedent for using the term "strong typedef" for this kind of thing: `LLVM_YAML_STRONG_TYPEDEF`. Might not matter if the goal is to eventually replace LLVM_YAML_STRONG_TYPEDEF with this, but otherwise, it would be nice to maintain consistent naming within the project.
I'm totally fine changing the name.  Other people objected to "strong typedef" and "semantic typedef" seemed the best proposed option to me.

The C++ committee also rejected the same "strong typdef," opting for "opaque typedef" for reasons that are not entirely clear to me.  I didn't want to name this `OpaqueTypedef` to avoid confusion with the language proposal of that name.


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


================
Comment at: llvm/include/llvm/ADT/SemanticTypedef.h:125
+
+  friend void swap(SemanticTypedef &A, SemanticTypedef &B) {
+    using std::swap;
----------------
nliber wrote:
> This needs an exception specification.
What should it be?  `is_nothrow_swappable` is only available in C++ 17.  Mkaing it unconditionally `noexcept` would be wrong.


================
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) {             \
----------------
nliber wrote:
> 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.
If people feel strongly about it, I'm fine not using macros.  It will be a lot more code, though.


================
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)));   \
----------------
nliber wrote:
> Why the casts?
That's a mistake.  Will fix.


================
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) */                                      \
----------------
nliber wrote:
> 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.
Good point.  Will fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66148