[PATCH] D31917: Add unique_dyn_cast

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Can the pieces of the test case be grouped together (currently it's classes, then some other things, then traits, then some other things, then the tests that use them)?

Also - having bits of one part of the test feed into another otherwise independent part of the test seems a bit sub-optimal. (I realize you're using ASSERT_* so if the first fails tehre won't be spurious failures in the latter tests - but it means you only get the first result from the test and nothing else) - I'd suggest splitting it up to at least be independent, and possibly to be in separate test functions so they can report independent results, etc.



================
Comment at: llvm/include/llvm/Support/Casting.h:363
+// taking ownership of the input pointer iff isa<X>(Val) is true.  If the
+// cast is successful, From refers to nullptr on exit and the casted value
+// is returned.  If the cast is unsuccessful, the function returns nullptr
----------------
I'd probably phrase this as "From is null on exit". (nullptr is a specific null literal, the concept of nullness is independent of nullptr (or NULL, or 0, etc) and "refers to null" seems a bit awkward/inaccurate because it doesn't refer to anything)


================
Comment at: llvm/unittests/Support/Casting.cpp:47
+
+struct derived : public base {
+  static bool classof(const base *B) { return true; }
----------------
you can drop the "public" in "public base" here, since it's a struct and has default public visibility


================
Comment at: llvm/unittests/Support/Casting.cpp:52
 template <> struct isa_impl<foo, bar> {
   static inline bool doit(const bar &Val) {
     dbgs() << "Classof: " << &Val << "\n";
----------------
'inline' isn't needed here


================
Comment at: llvm/unittests/Support/Casting.cpp:59
+template <typename T> struct isa_impl<foo, T> {
+  static inline bool doit(const T &Val) { return false; }
+};
----------------
'inline' isn't needed here


https://reviews.llvm.org/D31917