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

[Duncan P. N. Exon Smith via Phabricator via libcxx-commits]

dexonsmith added inline comments.


================
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:
> > 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92571