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

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

sgraenitz marked 3 inline comments as done.
sgraenitz added a comment.

> The part I'm not sure about is whether the RichManglingInfo vs. RichManglingSpec distinction brings any value. [...] So you might as well just have one class, pass that to DemangleWithRichManglingInfo, and then query the same object when the call returns.

The idea here was that `DemangleWithRichManglingInfo()` acts like a gate keeper. If it succeeds it provides read-only access to the updated `RichManglingInfo` in `RichManglingSpec`, otherwise it returns null. IMHO the value behind it is that `RichManglingInfo` does not need to handle a `NoInfo` case next to `ItaniumPartialDemangler` and `PluginCxxLanguage` in every single getter. Instead it's just not accessible in that state. (Plus: there is no maintenance functions that confuse the public interface of `RichManglingInfo`.) I don't know how to do this with only one class.

Maybe `RichManglingSpec` is not the perfect name. What about renaming it to `RichManglingContext`?

> I mean, the lifetime of the first is tied to the lifetime of the second, and the Spec class can only have one active Info instance at any given moment.

Yes, this was handy and avoids extra heap-allocations. `const RichManglingInfo *` was intended to clarify: lifetimes are handled elsewhere.

> The current interface with createItaniumInfo et al. makes it seem like one could call it multiple times in sequence, stashing the results, and then doing some post-processing on them.

Yes, I can see that this is implicit knowledge. Do you have an idea how to make this more explicit? Rename to `SetItaniumInfo()` maybe?

In the end, I definitely prefer this approach over having a `NoInfo` state in `RichManglingInfo`. What do you think?


https://reviews.llvm.org/D49990