lemo added inline comments.
================
Comment at: include/lldb/Core/Module.h:779
+ //------------------------------------------------------------------
+ void SetUUID(const lldb_private::UUID &uuid);
+
----------------
labath wrote:
> lemo wrote:
> > labath wrote:
> > > amccarth wrote:
> > > > Was there no SetUUID before because the UUID was considered immutable?
> > > > I wonder if that's worth keeping. Is the UUID always known at
> > > > construction time?
> > > The UUID is not always know at construction time -- sometimes it can be,
> > > if you set it on the FileSpec you use to create the module, but you don't
> > > have to do that.
> > >
> > > However, it is true that the UUID was constant during the lifetime of the
> > > Module, and this changes that, which is not ideal IMO.
> > >
> > > It seems that in the only place you call this function, you already know
> > > the UUID. Would it be possible to do this work in the PlaceholderModule
> > > constructor (thereby avoiding any changes to the Module API) ?
> > Good point: I agree, it's a good idea to preserve the invariant that the
> > module UUID does not change once it's set.
> >
> > I'd not add more complexity & arguments to the placeholder module
> > constructor though. Moreover, even if it's technically possible to reach
> > into the protected data members from Module I'd like to avoid doing that
> > since Module::m_uuid is closely tied to Module::m_mutex and
> > Module::m_did_parse_uuid (the later now renamed to m_did_set_uuid).
> >
> > I added a check and assert to prevent Module::SetUUID from overwriting an
> > already set UUID. (Personally I'd would've liked to have the assert be a
> > fastfail in all builds but I remember that is somewhat against the LLDB
> > philosophy)
> >
> >
> Guarding the invariant with an assert is better than nothing, but even better
> is arranging stuff so that the invariant cannot be broken in the first place.
> And this feels like the sort of thing that should be possible to make safe by
> design.
>
> How about adding a special (protected?) Module constructor, which takes a
> UUID argument and uses it to initialize the m_uuid field. This way, it is
> impossible by design to change the UUID mid-flight, and you still don't have
> to reach into the protected Module fields from the other class (we could even
> make them private if we want to).
Trying to make sure that the invariant can't be invalidated is a high bar
considering that most Module state is protected. But I agree with the sentiment
and here's a new iteration:
1. Made Module::SetUUID() protected
(note that the m_uuid change is conditional, not just protected by an
lldbassert)
2. PlaceholderModule constructor is where SetUUID is called from
I'm hesitant to add another Module constructor: that class already has 2 public
+ 1 private constructors (even using a delegating constructor, the addition of
a new protected constructor questionable). But if you still prefer that option
better I'll make the change.
https://reviews.llvm.org/D46292
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
