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

[Zachary Turner]

Also, I know we started the discussion here, but perhaps it's better moved
to the other thread that has the latest review, as this one is essentially
abandoned.


On Thu, Sep 4, 2014 at 6:35 PM, Zachary Turner <zturner at google.com> wrote:

> 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'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/15e44434/attachment.html>