[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

It sounds like we have a plan then! Look forward to seeing the results.



================
Comment at: source/Core/RichManglingInfo.cpp:36-40
+RichManglingContext::SetLegacyCxxParserInfo(const ConstString &mangled) {
+  m_info.ResetProvider();
+  m_info.m_provider = RichManglingInfo::PluginCxxLanguage;
+  m_info.m_legacy_parser = new CPlusPlusLanguage::MethodName(mangled);
+  return &m_info;
----------------
sgraenitz wrote:
> labath wrote:
> > sgraenitz wrote:
> > > labath wrote:
> > > > Is this really the **mangled** name?
> > > Yes, it is.
> > How is that possible? I see nothing in CPlusPlusLanguage::MethodName which would perform the demangling, and yet it clearly operates on demangled names. Also, the LHS of this patch creates MethodName instances like this
> > `CPlusPlusLanguage::MethodName cxx_method(mangled.GetDemangledName(lldb::eLanguageTypeC_plus_plus));`
> > 
> > 
> Oh right, I should revisit that.
> 
> While we are here: It's a pity that only MSVC-mangled names trigger the fallback case with the new implementation. Thus, my unit test has no coverage for it and I didn't recognize the mistake. I thought about integrating ObjCLanguage parsing here too. On the one hand this would improve test coverage. On the other hand it would cause a semantic change in name indexing (and my ObjC knowledge is quite limited).
> 
> What do you think?
I understand Zachary is working on an MSVC demangler as we speak. When that's finished, we should be able to add a couple of MSVC mangled names to your test case. (The `MethodName` class itself is tested elsewhere, so here we just need to test that things are wired up correctly.)

As for the ObjC idea, I am not sure what exactly would that entail, so I don't have an opinion on it. But I can certainly believe that there is a lot of room for improvement here.


https://reviews.llvm.org/D50071