Page MenuHomePhabricator

[lldb] Decouple ObjCLanguage from Symtab
Needs ReviewPublic

Authored by bulbazord on Thu, Jun 10, 2:59 PM.

Details

Reviewers
teemperor
Summary

We can extend/modify GetMethodNameVariants to suit our purposes here.
What symtab is looking for is alternate names we may want to use to
search for a specific symbol, and asking for variants of a name makes
the most sense here.

It might make more sense to wrap the ConstString and FunctionNameType
into a struct with a name, but for now I think a pair suffices.

Diff Detail

Event Timeline

bulbazord created this revision.Thu, Jun 10, 2:59 PM
bulbazord requested review of this revision.Thu, Jun 10, 2:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jun 10, 2:59 PM

Note that this is working towards solving the same problem as D103210 but it does so in a different way. Instead of repurposing the old diff, I made a new one so it is a little easier to compare/contrast. Hopefully it's not too confusing.

This looks pretty good to me.

It's a little awkward in InitNameIndexes that we look up the various NameToSymbolIndex maps by eFunctionNameType, use the function name type again to sort the names & index pairs into the bucket we looked up before. I wonder if that could be made cleaner by having an

AddToSymbolNameToIndexMap(symbol_name, index, func_name_type)

interface, which would just sort the symbol names into the right map. Not sure that's worth the bother, however.

lldb/source/Symbol/Symtab.cpp
345–346

Shouldn't this be in a loop over the supported languages?

teemperor requested changes to this revision.Fri, Jun 11, 12:38 AM

This looks pretty good to me.

It's a little awkward in InitNameIndexes that we look up the various NameToSymbolIndex maps by eFunctionNameType, use the function name type again to sort the names & index pairs into the bucket we looked up before. I wonder if that could be made cleaner by having an

AddToSymbolNameToIndexMap(symbol_name, index, func_name_type)

interface, which would just sort the symbol names into the right map. Not sure that's worth the bother, however.

That sounds good to me as a follow-up refactoring.

Only some small complains but otherwise this seems pretty good. Someone (*puts finger on nose*) should maybe do some stats whether

lldb/source/Breakpoint/BreakpointResolverName.cpp
222–223

Shouldn't that use the type form the variant instead of the name_type_mask? FWIW, I would prefer if we keep this code unchanged as right now we introduce the selectors to this list of lookups.

So what about adding filter here for eFunctionNameTypeFull to keep this patch NFC? And then maybe a FIXME: to figure out if we should add variants that aren't the full name.

lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
104

Could we make this a custom struct? first second are always so non-descriptive and we might want to extend what we return from this function in a future patch.

class MethodNameVariant {
  ConstString m_name;
  lldb::FunctionNameType m_type;
public:
  [...]
}
lldb/source/Symbol/Symtab.cpp
345–346

+1

I also wonder if this might become quite expensive in the future if we end up with more language plugins, but I guess in that case we can make some kind of initial query pass where plugins can register whether they care about this indexing stuff here. Anyway, I don't think that's a real concern at the moment.

This revision now requires changes to proceed.Fri, Jun 11, 12:38 AM
shafik added a subscriber: shafik.Fri, Jun 11, 11:29 AM
shafik added inline comments.
lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
104

I second (pun intended?) this, if we can avoid using std::pair and instead use a custom struct we should.

bulbazord updated this revision to Diff 351696.Sat, Jun 12, 7:58 PM

Addressing comments