[libcxx-commits] [PATCH] D92571: [IR] Remove CastInst::isCastable since it is not used

[Cullen Rhodes via Phabricator via libcxx-commits]

c-rhodes marked an inline comment as done.
c-rhodes added a comment.

In D92571#2431860 <https://reviews.llvm.org/D92571#2431860>, @dexonsmith wrote:

> LGTM too if you drop the libcxxabi change.
>
> If there's an out-of-tree user that needs this we can revert again if it makes sense to (in which case, it'd be better to change `getCastOpcode` to return an `Optional<Instruction::CastOps>`, so we're not having to keep two functions in sync like this...).

SGTM, I'll land it early next week in-case of any fallout, thanks for reviewing.



================
Comment at: libcxxabi/test/test_demangle.pass.cpp:21547
     {"_ZN4llvm8CastInst12CreateFPCastEPNS_5ValueEPKNS_4TypeERKNS_5TwineEPNS_10BasicBlockE", "llvm::CastInst::CreateFPCast(llvm::Value*, llvm::Type const*, llvm::Twine const&, llvm::BasicBlock*)"},
-    {"_ZN4llvm8CastInst10isCastableEPKNS_4TypeES3_", "llvm::CastInst::isCastable(llvm::Type const*, llvm::Type const*)"},
     {"_ZN4llvm8CastInst13getCastOpcodeEPKNS_5ValueEbPKNS_4TypeEb", "llvm::CastInst::getCastOpcode(llvm::Value const*, bool, llvm::Type const*, bool)"},
----------------
ldionne wrote:
> dexonsmith wrote:
> > ldionne wrote:
> > > dexonsmith wrote:
> > > > I don't understand why this is being removed. Is the demangler testcase no longer valid?
> > > It's still valid -- the only reason why I'm not opposing to the removal is that I can see a *small* benefit in not leaving behind old definitions like that, which show up whenever you grep for the function name in the whole monorepo.
> > > 
> > > Basically, I don't mind whether we keep it or not. I think the benefit is small, but the loss of test coverage is also very small, since I assume those symbols were just taken more or less randomly and passed through `c++filt`.
> > Yeah, I would have to guess that all the symbols in an LLVM binary were demangled with our previous system demangler, to add as testcases for libc++abi.
> > 
> > My inclination is that it's better to leave it behind, until / unless the demangler tests are rethought as a whole in some way (to get similar coverage another way).
> > 
> > I agree that this file coming up in `git grep` is a bit annoying (it happens a lot to me)... but since this patch deletes `CastInst::isCastable` I expect this line won't bother anyone again (since no one will be looking for the deleted function). It's all the other lines that will continue to cause friction.
> > 
> > I also worry that if we start deleting this one at a time, we'll add a bunch of uninteresting commits to:
> > ```
> > % git log -- libcxxabi/
> > ```
> > 
> > WDYT?
> Ok, I'm fine with your suggestion. @c-rhodes , can you please leave that test in libc++abi? And then libc++abi won't be affected by this change, so you still have my LGTM :-).
> Ok, I'm fine with your suggestion. @c-rhodes , can you please leave that test in libc++abi? And then libc++abi won't be affected by this change, so you still have my LGTM :-).

np cheers, I've removed the change.


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

https://reviews.llvm.org/D92571