<div dir="ltr">Yes, sorry for the confusion.  There's a totally new review up.  I commented in this thread yesterday saying that I uploaded a new review, but since it was completely different it was in a new issue entirely.  Then comments about that review started coming in on this thread.  Anyway, look here: <a href="http://reviews.llvm.org/D5198">http://reviews.llvm.org/D5198</a></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Sep 5, 2014 at 4:35 PM, Todd Fiala <span dir="ltr"><<a href="mailto:tfiala@google.com" target="_blank">tfiala@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hey Zachary - am I looking in the right spot?  The top of the thread is talking about this:<div><br></div><div><a href="http://reviews.llvm.org/D5110" target="_blank">http://reviews.llvm.org/D5110</a><br></div><div><br></div><div>You said this:</div><span class=""><div><br></div><div>> <span style="font-family:arial,sans-serif;font-size:12.7272720336914px">Latest version is up, should address all the issues pointed out in yoru comments.  </span></div><div><br></div></span><div>Which leads me to believe I would see multiple versions of it.  I'm only seeing the August patch on <a href="http://reviews.llvm.org" target="_blank">reviews.llvm.org</a>.  What am I missing?  (I'm guessing it's the wrong review?)</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>-Todd</div></font></span></div><div class="gmail_extra"><br><br><div class="gmail_quote"><div><div class="h5">On Fri, Sep 5, 2014 at 3:31 PM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"><div dir="ltr">Latest version is up, should address all the issues pointed out in yoru comments.  I didn't actually address the HostThread::GetName() issue in this patch, because it turns out we only ever attempt to get the thread name in one place, and it's of the current thread.  I still am fine changing this, but the CL is already pretty huge and it didn't seem too urgent for the purposes of this change.</div><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Sep 5, 2014 at 9:17 AM,  <span dir="ltr"><<a href="mailto:jingham@apple.com" target="_blank">jingham@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Yes, the "alternate" method sounds fine to me as well.<br>
<span><font color="#888888"><br>
Jim<br>
</font></span><div><div><br>
<br>
> On Sep 4, 2014, at 6:35 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
><br>
> 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.<br>
><br>
><br>
> On Thu, Sep 4, 2014 at 6:18 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
><br>
><br>
><br>
> On Thu, Sep 4, 2014 at 6:06 PM, <<a href="mailto:jingham@apple.com" target="_blank">jingham@apple.com</a>> wrote:<br>
><br>
> > On Sep 4, 2014, at 5:57 PM, Zachary Turner <<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>> wrote:<br>
> ><br>
> > 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.<br>
><br>
> 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.<br>
><br>
> 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.<br>
><br>
> lldb::thread_t backup = m_thread;<br>
> ...<br>
> m_thread = backup;<br>
><br>
> With my patch, that becomes the following:<br>
><br>
> HostThread backup = m_thread;<br>
> ...<br>
> m_thread = backup;<br>
><br>
> With the base class method, that becomes the following:<br>
><br>
> HostThreadBaseSP backup = m_thread->Clone();<br>
> ...<br>
> m_thread = backup->Clone();<br>
><br>
> 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.<br>
><br>
> 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.<br>
><br>
> 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.<br>
><br>
> This way, in generic code, as long as you don't write thread.GetNativeThread(), you're guaranteed to only be using generic methods.<br>
><br>
<br>
</div></div></blockquote></div><br></div>
</div></div><br></div></div><span class="">_______________________________________________<br>
lldb-commits mailing list<br>
<a href="mailto:lldb-commits@cs.uiuc.edu" target="_blank">lldb-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits</a><br>
<br></span></blockquote></div><br><br clear="all"><span class=""><div><br></div>-- <br><div dir="ltr"><table cellspacing="0" cellpadding="0" style="color:rgb(136,136,136);font-family:'Times New Roman'"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top-style:solid;border-top-color:rgb(213,15,37);border-top-width:2px">Todd Fiala |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(51,105,232);border-top-width:2px"> Software Engineer |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(0,153,57);border-top-width:2px"> <a href="mailto:tfiala@google.com" style="color:rgb(17,85,204)" target="_blank"><span style="background-color:rgb(255,255,204);color:rgb(34,34,34);background-repeat:initial initial">tfiala@google.com</span></a> |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(238,178,17);border-top-width:2px"><font color="#1155cc"> <a>650-943-3180</a></font></td></tr></tbody></table><br></div>
</span></div>
</blockquote></div><br></div>