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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits