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
_______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
