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

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added inline comments.


================
Comment at: include/lldb/Core/RichManglingInfo.h:36
+  /// "a::b".
+  llvm::StringRef GetFunctionDeclContextName() const;
+
----------------
I realized that returning `llvm::StringRef` from here may not be the best idea. To the outside it appears to be an immutable string, but internally it will be reused and even realloc'ed. I guess returning a `ConstString` is not acceptable as an alternative performance-wise, as it adds one `memcpy` per query. I will sketch something here, but it's not easy as it needs to fit both, the IPD and C++ MethodName.


================
Comment at: include/lldb/Core/RichManglingInfo.h:61
+    assert(m_legacy_parser.hasValue());
+    assert(llvm::any_isa<ParserT>(m_legacy_parser));
+    return llvm::any_cast<ParserT *>(m_legacy_parser);
----------------
This must be `llvm::any_isa<ParserT *>`. Found during fallback-test and fixed.


================
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;
----------------
labath wrote:
> 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.
It's actually only a few lines to hack, so that the unit test covers the fallback case. I did it once now and found the `llvm::any_isa<ParserT>` bug. Otherwise it passes.


https://reviews.llvm.org/D50071