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

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

sgraenitz marked 2 inline comments as done.
sgraenitz added inline comments.


================
Comment at: include/lldb/Core/RichManglingInfo.h:83-84
+public:
+  RichManglingInfo *SetItaniumInfo();
+  RichManglingInfo *SetLegacyCxxParserInfo(const ConstString &mangled);
+
----------------
labath wrote:
> I find it odd that one of these functions is stateless (`SetLegacyCxxParserInfo` takes the string which initializes the object as an argument), while the other receives the state magically via the IPD accessor.
> 
> Would it be possible to change it so that the other option is stateless too (`SetItaniumInfo(ConstString mangled)`)?
> 
> We could change `RichManglingInfo` to return the full demangled name too, so that you have access to the demangled name via this API too (which /I think/ is the reason why you created a separate `GetItaniumRichDemangleInfo` function).
> Would it be possible to change it so that the other option is stateless too (`SetItaniumInfo(ConstString mangled)`)?
I think that would be misleading for the reader. Symmetry is always nice to indicate congruent behavior, but this is not the case here. The fallback C++ parser version mimics the previous behavior: demangle the name in step 1 and if that succeeds, parse it with `CPlusPlusLanguage::MethodName` to decode its properties. The IPD does that in a single step and so there is nothing to do in `SetItaniumInfo()`. `SetLegacyCxxParserInfo()` on the other hand has to create a `CPlusPlusLanguage::MethodName` from the mangled string.

> We could change `RichManglingInfo` to return the full demangled name too, so that you have access to the demangled name via this API too (which /I think/ is the reason why you created a separate GetItaniumRichDemangleInfo function).
The interface wants to provide access to the name's encoded properties. That doesn't really include the demangled "human readable" string representation (it's also not needed in `Symtab::RegisterMangledNameEntry()`).

Of course `Symtab::InitNameIndexes()` will ask for `GetDemangledName()` just after processing the mangled one. I think chances are good that for symbols that are not filtered out anyway, we reached `DemangleWithRichManglingInfo()` earlier and it will be in the string pool already (see Mangled.cpp:322). Otherwise, yes it will be demangled in with a new IPD instance.

I think it's not bad to keep these cases as they are. I would propose to investigate the performance of the current implementation in more detail and think about the benefits before mixing them. Not sure at all, if it would bring measurable gain.


================
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:
> Is this really the **mangled** name?
Yes, it is.


https://reviews.llvm.org/D50071