Page MenuHomePhabricator

[lldb] Introduce Language::MethodNameInfo
Needs ReviewPublic

Authored by bulbazord on Wed, May 26, 3:02 PM.

Details

Reviewers
teemperor
Summary

There are several instances where one may want to ask questions about
the name of a method/function/whatever. Classes like
CPlusPlusLanguage::MethodName and ObjCLanguage::MethodName fulfill this
purpose right now, but users of these have to have knowledge on these
classes (from plugins) to function. I propose a new class
Language::MethodNameInfo to abstract this away so that lldb's Core
libraries can ask questions about a method name without knowing the
details of a plugin.

Currently the class only has one method to override, GetSelector, but
I imagine it will have more in the future. I only added the one method
needed to replace the use of ObjCLanguage in Symtab.

Diff Detail

Event Timeline

bulbazord requested review of this revision.Wed, May 26, 3:02 PM
bulbazord created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptWed, May 26, 3:02 PM

Let me know how you feel about this abstraction/idea. I think we'll need something like this at some point to remove most of the remaining dependencies (e.g. in Module.cpp) so I'd like to get something good going here.

The idea seems good, but I'm not sure how you are going to extend it? "Selector" seems a pretty ObjC specific term to me. Will this be what you use to get the "method name" part of the C++ names? That would look a bit odd to me, but OTOH having a GetSelector in all the subclasses, most of which don't have selectors also seems a little odd. This might be solved by just thinking of a better name, however. Also note, in the C++ case, it would be useful to distinguish between the method name, like Bar in Foo::Bar, and the fully qualified method name (like Bar(int, double) in Foo::Bar(int, double). That might effect the choice of names. The selector is the equivalent of a C++ fully qualified method name.

+1 to not add any language-specific terms to the Language API.

Looking over the code, it seems what the code is doing its:

  • Going over the symbol names.
  • Creating a search index from the symbol names that is optionally grouped via the FunctionNameType cases (and there is a 'eFunctionNameTypeSelector' which isn't that nice but that's why we try to do selector-specific stuff).

So what about having an API that for a given symbol name gives us all the alternative names we should put into this search index (and optionally a FunctionNameType so we can put it into the right group)? I guess C++ could also benefit from the same API as it would just return the right basenames then?

If we make the different m_*to_index maps in Symtab into something two nested maps with the form FunctionNameType -> name -> index, then that would also remove quite a bit of copy-pasted code in Symtab.cpp

+1 to not add any language-specific terms to the Language API.

Looking over the code, it seems what the code is doing its:

  • Going over the symbol names.
  • Creating a search index from the symbol names that is optionally grouped via the FunctionNameType cases (and there is a 'eFunctionNameTypeSelector' which isn't that nice but that's why we try to do selector-specific stuff).

So what about having an API that for a given symbol name gives us all the alternative names we should put into this search index (and optionally a FunctionNameType so we can put it into the right group)? I guess C++ could also benefit from the same API as it would just return the right basenames then?

We have a method called Language::GetMethodNameVariants, that could be extended/modified to be more suitable. Unless you had something else in mind? Maybe something more specific to Symtab mapping like Language::GetFunctionNameSearchVariants.

If we make the different m_*to_index maps in Symtab into something two nested maps with the form FunctionNameType -> name -> index, then that would also remove quite a bit of copy-pasted code in Symtab.cpp

This makes sense to me, I'll also make this change in this patch :)

Sorry for the delay!

We have a method called Language::GetMethodNameVariants, that could be extended/modified to be more suitable. Unless you had something else in mind? Maybe something more specific to Symtab mapping like Language::GetFunctionNameSearchVariants.

That actually seems like a great idea! Module::LookupInfo also seems quite close to the data structure we would need to return, so we could in theory return a list of that from the function instead.

This makes sense to me, I'll also make this change in this patch :)

Feel free to also put it up as a NFC patch. It seems in general like a good cleanup from what I can see, so that should be a quick review :)

We have a method called Language::GetMethodNameVariants, that could be extended/modified to be more suitable. Unless you had something else in mind? Maybe something more specific to Symtab mapping like Language::GetFunctionNameSearchVariants.

That actually seems like a great idea! Module::LookupInfo also seems quite close to the data structure we would need to return, so we could in theory return a list of that from the function instead.

Awesome, I'll work on that next :)

This makes sense to me, I'll also make this change in this patch :)

Feel free to also put it up as a NFC patch. It seems in general like a good cleanup from what I can see, so that should be a quick review :)

Just uploaded: D103652