[Lldb-commits] [PATCH] Implements a HostThread class.

[Zachary Turner]

On Thu, Sep 4, 2014 at 6:06 PM, <jingham at apple.com> wrote:

>
> > On Sep 4, 2014, at 5:57 PM, Zachary Turner <zturner at google.com> wrote:
> >
> > Regarding point #1: I'm still not sold on this.  Exposing only the
> HostThreadBase really complicates some things.  Some of the issues escape
> my mind right now, but one in particular stands out.  There are a couple of
> places where threads are copied.  I don't remember the exact file off the
> top of my head, but something about making a copy of a thread in case the
> thread is shutting down and nulls itself out.  With a pointer or reference
> to a base class, this can't be done without a virtual Clone() method, which
> is really kind of gross.
>
> I would like to see what is hard.  You are using a generic factory to make
> the HostThreads, your ThreadRunner, and then you are calling generic
> methods on them.  Maybe I'm too stuck on this, but it seems like we should
> keep host specific stuff out of generic lldb functionality, and enforce
> that with the compiler, not with buildbots on the lacking host failing
> sometime later on...  That just seems ugly to me.


It's not that it's hard, I guess it's just a difference of opinion on
what's the most ugly.  Consider the following block of code which uses raw
lldb:thread_t's.

lldb::thread_t backup = m_thread;
...
m_thread = backup;

With my patch, that becomes the following:

HostThread backup = m_thread;
...
m_thread = backup;

With the base class method, that becomes the following:

HostThreadBaseSP backup = m_thread->Clone();
...
m_thread = backup->Clone();

These look similar, but behind the scenes it's ugly.  You now need a pure
virtual Clone() method on HostThreadBase (trivial to implement of course,
but it's just code pollution).  You need to store by pointer instead of by
value, which means you have to worry about null-checks as well.

There is the issue you mention which is that only a buildbot will tell you
if you use a platform-specific method, but my argument is just that this is
strictly better than before, because before *nobody* would tell you when
you used a platform-specific method.  It would just be a no-op.

That said, I have another idea in case you're still opposed to the way I've
done it.  Basically, just have HostThread.  Nothing derives from it.  Its
only member is a shared_ptr<HostNativeThread>.  HostNativeThread does have
subclasses just as the current HostThreadBase does.  The methods of
HostThread just forward their calls to HostNativeThread, but also
HostThread provides a method called GetNativeThread() which returns
HostNativeThread automatically cast to the most-derived type.

This way, in generic code, as long as you don't write
thread.GetNativeThread(), you're guaranteed to only be using generic
methods.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140904/d107b13a/attachment.html>