[PATCH] D147991: [LLVM][Casting.h] Fix dyn_cast for std::unique_ptr.

[Aman LaChapelle via Phabricator via llvm-commits]

bzcheeseman accepted this revision.
bzcheeseman added a comment.

@zuban32 Not sure if you were waiting on me (if so, sorry!) - I put some comments inline that would help with the concerns we talked about previously. I'd love to get this landed once the comments are resolved :)



================
Comment at: llvm/unittests/Support/Casting.cpp:231
+  EXPECT_EQ(DP.get(), nullptr);
+  EXPECT_NE(BP2.get(), nullptr);
+  BP2.release();
----------------
Would it make sense to check something like
```
base *BP2Ptr = BP2.get();
EXPECT_NE(BP2Ptr, nullptr);
auto DP = dyn_cast<derived_nocast>(std::move(BP2));
EXPECT_EQ(DP.get(), nullptr);
EXPECT_NE(BP2.get(), nullptr);
EXPECT_EQ(BP2.get(), BP2Ptr);
```
?

That would assuage my concern, I think, as this at least ensures we have the exact behavior we expect on failure.


================
Comment at: llvm/unittests/Support/Casting.cpp:232
+  EXPECT_NE(BP2.get(), nullptr);
+  BP2.release();
 }
----------------
I don't believe this is necessary? I think we want it to be deallocated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147991