Page MenuHomePhabricator

[lldb] Remove modern-type-lookup
ClosedPublic

Authored by teemperor on Dec 16 2019, 11:51 AM.

Details

Summary

As discussed on the mailing list [1] we have to make a decision for how to proceed with the modern-type-lookup.

This patch removes modern-type-lookup from LLDB. This just removes all the code behind the modern-type-lookup
setting but it does *not* remove any code from Clang (i.e., the ExternalASTMerger and the clang-import-test stay around
for now).

The motivation for this is that I don't think that the current approach of implementing modern-type-lookup
will work out. Especially creating a completely new lookup system behind some setting that is never turned on by anyone
and then one day make one big switch to the new system seems wrong. It doesn't fit into the way LLVM is developed and has
so far made the transition work much more complicated than it has to be.

A lot of the benefits that were supposed to come with the modern-type-lookup are related to having a better organization
in the way types move across LLDB and having less dependencies on unrelated LLDB code. By just looking at the current code (mostly
the ClangASTImporter) I think we can reach the same goals by just incrementally cleaning up, documenting, refactoring
and actually testing the existing code we have.

[1] http://lists.llvm.org/pipermail/lldb-dev/2019-December/015831.html

Diff Detail

Event Timeline

teemperor created this revision.Dec 16 2019, 11:51 AM

@martong I added you as a reviewer because I assume you were interested in that development. I'm curious what you think should happen to the clang-import-test. We could either rewrite the tests as unit tests in the ASTImporterTest you guys are already using or we move the necessary parts of the ExternalASTMerger into the clang-import-test (which will mostly likely just be the lookup code). I leave it up to you guys to decide what should happen with that testing code.

I'm landing this now as I started the cleanup work for the current lookup code and the modern-type-lookup code is blocking that refactoring.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 17 2019, 3:28 AM
This revision was automatically updated to reflect the committed changes.

I'm curious what you think should happen to the clang-import-test. We could either rewrite the tests as unit tests in the ASTImporterTest you guys are already using or we move the necessary parts of the ExternalASTMerger into the clang-import-test

Raphael, I think there is no point to keep the clang-import-test and the ExternalASTMerger in Clang if we remove the only user of these things.
So, in my opinion the best would be on a long-term if we could cover these tests with unit tests in ASTImporterTest.
On the other hand, I understand that rewriting these tests could be quite a work, so perhaps we should gradually add the new unit tests and once we are ready then we could remove entirely the clang-import-test and the ExternalASTMerger.
Also, some of the tests are already covered by the existing unit tests, e.g. switch-stmt with ImportSwitch.