<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Aug 28, 2014 at 7:58 PM, <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">I guess I view this the other way round from you.<br>
<br>
The host layer has two sides to it. To me the primary one is to provide an interface that isolates generic lldb code from the underlying details of the host system that it is compiled for. Secondarily it will provide interfaces that the various bits of the host layer for that particular host use to implement the generic Host layer.<br>
<br>
It seems to me you are saying the job of providing a generic host layer for lldb to use is too difficult and so when generic lldb code is using the host layer to make a new thread, or manipulate a file, or launch a process, it has really got to know it is using host specific functionality whose specific details it might at any moment have to be aware of. That just seems wrong to me. If I'm using some Host provided capability in Process, or ProcessGDBRemote or when dealing with FileSpec's anywhere in the Dwarf reading code or whatever I can't have to know what host I'm actually on. Otherwise you're going to have to start intruding host specific details into generic lldb algorithms.<br>
<br>
So it seems to me precisely the opposite of what you are claiming, if I have to have my hands on the Host specific version of any of the Host classes we are providing in Process, etc, then the host layer is a failed abstraction. I don't think that is necessary...<br>
</blockquote><div>I'm not saying you have to, I'm saying there's no reason not to, because for any given build configuration, there will just be 1 HostThread class. What's the point of passing around a HostThreadBase everywhere, when the only thing that derives from it is a HostThreadMacOSX?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
><br>
> It is clear, but I don't think I agree with it.<br>
><br>
> The overhead doesn't worry me, few if any of the host methods are going to be used in a tight loop where the cost of virtual methods will actually matter.<br>
><br>
> And I'm not sure I buy the "casting" awkwardness argument all that much, since for the most part the Host specific parts of the HostXXX implementations are going to be in the methods that implement each class - operating on "this" which is already of the full host specific type. The number of times I will have to reach into the Host specific methods of an object not the one I'm implementing doesn't seem likely to be all that great.<br>
><br>
> I agree that the performance overhead of virtual methods isn't significant. On the other hand, it feels strange to use a language feature which changes the way code is generated just for the purposes of documentation. That's usually what comments are for. That said, I don't feel that strongly about it, so I'm willing to make a base class with a common interface. It's surprisingly difficult to make a useful common interface for a thread though, because even methods that seem to make logical sense on all platforms are sometimes semantically different enough that it's difficult to find a function signature that has the stuff you need on every platform. But it's worth trying.<br>
<br>
> One thing I do feel kind of strongly about though is the idea of accessing this through a base class pointer or reference. Who wants to deal with a base class when you can have the derived class? It introduces extra, error-prone lines of code for no practical benefit.<br>
<br>
<br>
</div>I don't understand this comment. In generic code, like Process or ProcessGDBRemote where we need to spawn a thread to handle the private process events or the gdb-remote reading, or start up debugserver, why would I need full HostThreadXXX or HostProcessXXX object? The only reason would be because I'm going to have to do some:<br>
<br>
#if HOST_THREAD == HostThreadXXX<br>
<br>
stuff in generic code, but that means again the host abstraction has failed me. I don't want that...<br></blockquote><div>You probably wouldn't need it, although I can imagine some situations. These are not thread-related mind you, but off the top of my head, the public API currently exposes some methods that allow you to get the effective user id, group id, etc of the process. I don't want a method on HostInfoWindows called GetEffectiveUserId() that returns 0. It's a concept that doesn't exist. Likewise, you probably don't want a method on HostInfoMacOSX called GetCurrentUserDomainName(). Instead, in these exceptional cases where there's a public API that does platform specific stuff, I'm just saying you can move the pre-processor decision there. </div>
<div><br></div><div>Just to be clear though, there would not be any code that looks like this:</div><div><br></div><div>#if HOST_THREAD == HostThreadMacOSX</div><div>#endif</div><div><br></div><div>it would be this:</div>
<div><br></div><div>#if defined(__APPLE__)</div><div> thread.SomeAppleSpecificMethod();</div><div>#endif</div><div><br></div><div>This kind of code is already fairly prevalent in the codebase, and I don't anticipate that my changes as proposed will increase this kind of thing at all. Because even under the solution you're proposing with using HostThreadBase everywhere, it's still possible, but like this:</div>
<div><br></div><div>#if defined(__APPLE__)</div><div> ((HostThreadMacOSX&)thread).SomeAppleSpecificMethod();</div><div>#endif</div><div><br></div><div>It's the same thing, but just uglier. </div><div><br></div>
<div>FWIW, I think we are on the same page about wanting to keep pre-processor defines to a minimum in the codebase. That's actually a large part of my motivation for doing all this refactoring in the first place. If you take a look at the various HostInfo classes, that refactor removed almost 100% of preprocessor definitions that were in the original code. So I'm very cognizant of them and one of my explicit goals is getting rid of them.</div>
</div></div></div>