[Lldb-commits] [PATCH] D104067: [lldb] Decouple ObjCLanguage from Symtab

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

In D104067#2811834 <https://reviews.llvm.org/D104067#2811834>, @jingham wrote:

> 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



================
Comment at: lldb/source/Breakpoint/BreakpointResolverName.cpp:225
+      auto variant_name = variant_name_and_type.first;
       Module::LookupInfo variant_lookup(name, name_type_mask,
                                         lang->GetLanguageType());
----------------
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.


================
Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h:104
+  // We also return the FunctionNameType of each possible name.
+  std::vector<std::pair<ConstString, lldb::FunctionNameType>>
   GetMethodNameVariants(ConstString method_name) const override;
----------------
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.

```
lang=c++
class MethodNameVariant {
  ConstString m_name;
  lldb::FunctionNameType m_type;
public:
  [...]
}```


================
Comment at: lldb/source/Symbol/Symtab.cpp:332
         // name, add the version without categories to the index too.
-        ObjCLanguage::MethodName objc_method(name.GetStringRef(), true);
-        if (objc_method.IsValid(true)) {
-          selector_to_index.Append(objc_method.GetSelector(), value);
-
-          if (ConstString objc_method_no_category =
-                  objc_method.GetFullNameWithoutCategory(true))
-            name_to_index.Append(objc_method_no_category, value);
+        if (auto *objc_lang = Language::FindPlugin(lldb::eLanguageTypeObjC)) {
+          for (auto variant_name_and_type :
----------------
jingham wrote:
> Shouldn't this be in a loop over the supported languages?
+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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104067