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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think there is still something wrong with the diff. I can't see any of the callers of e.g. `createItaniumInfo` but I can see the function on both LHS and RHS of the diff (which shouldn't be the case as it's a new function). It looks like you uploaded just an ammending patch instead of the entire work. Can you fix that?

In https://reviews.llvm.org/D49990#1182003, @sgraenitz wrote:

> > 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?


Yes, I can see what you mean here. Neither of the solutions is particularly appealing. I guess if I were implementing this, I'd go with the "invalid state" option, though I am not sure why, as usually I am opposed to invalid states. Maybe we can leave this to the discretion of the implementor (you).



================
Comment at: include/lldb/Symbol/Symtab.h:81-83
+  // No move
+  RichManglingInfo(RichManglingInfo &&) = delete;
+  RichManglingInfo &operator=(RichManglingInfo &&) = delete;
----------------
sgraenitz wrote:
> labath wrote:
> > This is implied by the deleted copy operations.
> Which are implicitly deleted too, due to the existence of the destructor right? Does LLVM/LLDB have some kind of convention for it? I like to be explicit on ctors&assignment ("rule of 5"), because it aids error messages, but I would be fine with following the existing convention here.
As far as I know, the presence of a destructor has no impact on the state of copy/move operations, so you still need to delete the copy operations explicitly.

I don't know if there is an official policy on explicitly deleting move operations, but I don't remember seeing that style anywhere. However, I don't care much about that either.


https://reviews.llvm.org/D49990