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