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