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

[Alex Bezzubikov via Phabricator via llvm-commits]

zuban32 added inline comments.


================
Comment at: llvm/unittests/Support/Casting.cpp:231
+  EXPECT_EQ(DP.get(), nullptr);
+  EXPECT_NE(BP2.get(), nullptr);
+  BP2.release();
----------------
bzcheeseman wrote:
> 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.
I've changed the behavior to do the moving only after the casting is proved possible, I haven't noticed any changes in the actual behavior though, both the test above and the actual usecase I needed this for.


================
Comment at: llvm/unittests/Support/Casting.cpp:232
+  EXPECT_NE(BP2.get(), nullptr);
+  BP2.release();
 }
----------------
bzcheeseman wrote:
> zuban32 wrote:
> > bzcheeseman wrote:
> > > I don't believe this is necessary? I think we want it to be deallocated.
> > I was just following the chunk above, I think gtest reports some memory issue should we omit the `release()`. I don't remember why, let me take a look
> Ah interesting. Thanks for looking into it!
@bzcheeseman sorry for abandoning this for a while, just made the changes we discussed. release() above was needed because asan couldn't understand where the original pointer came from, after allocating it locally the ptrs are deleted just alright.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147991