Great thanks!
On Fri, Sep 5, 2014 at 4:42 PM, Zachary Turner <[email protected]> wrote: > 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: http://reviews.llvm.org/D5198 > > > On Fri, Sep 5, 2014 at 4:35 PM, Todd Fiala <[email protected]> wrote: > >> Hey Zachary - am I looking in the right spot? The top of the thread is >> talking about this: >> >> http://reviews.llvm.org/D5110 >> >> You said this: >> >> > Latest version is up, should address all the issues pointed out in >> yoru comments. >> >> Which leads me to believe I would see multiple versions of it. I'm only >> seeing the August patch on reviews.llvm.org. What am I missing? (I'm >> guessing it's the wrong review?) >> >> -Todd >> >> >> On Fri, Sep 5, 2014 at 3:31 PM, Zachary Turner <[email protected]> >> wrote: >> >>> 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. >>> >>> >>> On Fri, Sep 5, 2014 at 9:17 AM, <[email protected]> wrote: >>> >>>> Yes, the "alternate" method sounds fine to me as well. >>>> >>>> Jim >>>> >>>> >>>> > On Sep 4, 2014, at 6:35 PM, Zachary Turner <[email protected]> >>>> 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 <[email protected]> >>>> wrote: >>>> > >>>> > >>>> > >>>> > On Thu, Sep 4, 2014 at 6:06 PM, <[email protected]> wrote: >>>> > >>>> > > On Sep 4, 2014, at 5:57 PM, Zachary Turner <[email protected]> >>>> 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. >>>> > >>>> >>>> >>> >>> _______________________________________________ >>> lldb-commits mailing list >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits >>> >>> >> >> >> -- >> Todd Fiala | Software Engineer | [email protected] | 650-943-3180 >> > > -- Todd Fiala | Software Engineer | [email protected] | 650-943-3180
_______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
