[PATCH] D147991: [LLVM][Casting.h] Fix dyn_cast for std::unique_ptr.
Aman LaChapelle via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu May 11 09:48:28 PDT 2023
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
More information about the llvm-commits
mailing list