[Lldb-commits] [PATCH] D108395: [lldb] Delete IRExecutionUnit::CollectCandidateCPlusPlusNames

Fri Aug 20 13:33:00 PDT 2021

bulbazord added a comment.

In D108395#2957194 <https://reviews.llvm.org/D108395#2957194>, @teemperor wrote:

> I'm very supportive of the idea that we delete everything that isn't tested :) (and I'm only partly joking here).

I can sympathize, I'm only partly serious with this change! :D

> However removing this should cause a bunch of test failures on Linux as it effectively disables D70721 <https://reviews.llvm.org/D70721> (the test added in that patch was just a unit test that is preserved with this patch, but the bigger problem describes there will break a few tests calling constructors on Linux).

It doesn't cause test any test failures on my Linux machine unfortunately. I don't think this codepath is currently exercised by any tests. Certainly the `FindAlternateFunctionManglings` test in `CPlusPlusLanguageTest.cpp` also uses `CPlusPlusLanguage::FindAlternateFunctionManglings`, but that's not the same thing as testing `IRExecutionUnit::CollectCandidateCPlusPlusNames` being tested as a part of `IRExecutionUnit::FindSymbol`.

> I believe the usual 'iterate over all language plugins' approach would again work here? We can guess from the mangled symbol what language we deal with (and in the worst case we could encode that in the IRExecutionUnit), so the plugins would just deliver alternative names. `GetAlternativeSymbolNames(ConstString symbol)` or something like that maybe?

I definitely think that the usual "iterate over all language plugins" approach would work here if we don't delete this (which it seems like we shouldn't). I mostly uploaded this change to see if anybody knew how to trigger this codepath. Before refactoring I'd like to write a test for this codepath, though I'm still not entirely sure how to do that yet.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108395