This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Refactor Module::LookupInfo constructor
ClosedPublic

Authored by bulbazord on Aug 17 2021, 11:55 AM.

Details

Summary

Module::LookupInfo's constructor currently goes over supported languages
trying to figure out the best way to search for a symbol name. This
seems like a great candidate for refactoring. Specifically, this is work
that can be delegated to language plugins.

Once again, the goal here is to further decouple plugins from
non-plugins. The idea is to have each language plugin take a name and
give you back some information about the name from the perspective of
the language. Specifically, each language now implements a
GetFunctionNameInfo method which returns an object of type
Language::FunctionNameInfo. Right now, it consists of a basename,
a context, and a FunctionNameType. Module::LookupInfo's constructor will
call GetFunctionNameInfo with the appropriate language plugin(s) and
then decide what to do with that information. I have attempted to maintain
existing behavior as best as possible.

A nice side effect of this change is that lldbCore no longer links
against the ObjC Language plugin.

Diff Detail

Event Timeline

bulbazord created this revision.Aug 17 2021, 11:55 AM
bulbazord requested review of this revision.Aug 17 2021, 11:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2021, 11:55 AM

Delete now unused variable

jingham accepted this revision.Aug 23 2021, 6:32 PM

I like the change in general, anywhere that we're explicitly listing languages in generic code we're probably doing it wrong... So far as I can tell you've reconstructed the old behaviors correctly in the new setup. I left one trivial comment, but really this LGTM.

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
73

The old code checked method.IsValid before pulling out the base name. It seems sensible that an invalid MethodName would return an empty base name, but that does seem to rely a little on implicit behavior? Not sure that's a big deal.

This revision is now accepted and ready to land.Aug 23 2021, 6:32 PM
bulbazord added inline comments.Aug 24 2021, 11:55 AM
lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
73

Yep, this does rely on implicit behavior. I don't think it's a huge deal, but it's easily changeable. :)

This revision was landed with ongoing or failed builds.Aug 24 2021, 1:54 PM
This revision was automatically updated to reflect the committed changes.

FYI (you may have have already noticed) this causes some merge conflicts in swift-lldb. It would be great if we could work together to figure out how to best resolve them.

FYI (you may have have already noticed) this causes some merge conflicts in swift-lldb. It would be great if we could work together to figure out how to best resolve them.

Yes, I figured that would happen at some point. I have an interest in seeing swift-lldb apply this change (and some of my other refactoring patches) so I would love to work with you and anyone else to get this change in smoothly. Unfortunately I had to revert this patch. It appears my environment had some issues that prevented me from running about half of lldb's test suite so I didn't catch any of the failures. I was finally able to debug the failing tests today and I realized that this change has several issues that need to be addressed. Once I have time to work through those issues and resubmit this patch, I'll add you as a reviewer and we can go from there. Does this sound good to you?