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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
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)"},
----------------
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 :-).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92571