[Lldb-commits] [PATCH] Implements a HostThread class.
zturner at google.com
Thu Sep 4 18:35:57 PDT 2014
Come to think of it I actually like my "alternate" method more than the way
I've done it, because it means I don't need to duplicate handles in the
assignment / copy-constructor of HostThreadWindows, because the only thing
that will get copied is the shared_ptr.
On Thu, Sep 4, 2014 at 6:18 PM, Zachary Turner <zturner at google.com> wrote:
> 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 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
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the lldb-commits