And FWIW, I have a patch locally which actually replaces all lldb::thread_t's with HostThreads. No pre-processor definitions crept in. ThreadJoin, Launch, and SetThreadName made up more than 95% of the conversions, and those are all part of the common interface.
On Thu, Aug 28, 2014 at 10:05 PM, Zachary Turner <[email protected]> wrote: > > > > On Thu, Aug 28, 2014 at 9:42 PM, Jason Molenda <[email protected]> wrote: > >> >> To take one small example, HostInfo::GetOSBuildString is defined for >> three out of the four platforms. And so any generic code that might want >> to call this has to do >> >> if (IsHost()) >> #if !defined(__linux__) >> return HostInfo::GetOSBuildString(s); >> #else >> return false; >> #endif >> > > But "os build string" and "os kernel version" are platform specific, by > definition. I find it really hard to agree that we should just allow any > old platform specific method to be exposed through a supposedly generic > interface with different subsets of methods broken on different subsets of > platforms. Then, you write some code to call a method like like > HostInfo::GetRedApples(), and you have no idea under what situations it's > even going to work. If a method isn't supported on your platform, you > shouldn't be trying to call it. If you are, that's already a good > indicator that you might have a layering problem. > > >> >> A call into *any* HostInfo method may similarly need to be #ifdef >> protected. And while it looks fine, to have #if !defined linux here, we >> know that eventually someone will add a method that only makes sense on Mac >> OS X (or so they think) and they'll do >> >> #if defined (__APPLE__) >> return HostInfo::GetRedApples(); >> #endif >> >> And then Solaris 16 will be released and suddenly it can get red apples >> too - a maintainer of the HostInfoSolaris class adds that method but then >> has to go find all the ifdef blocks and make them defined __APPLE__ || >> __SOLARIS__. > > >> >> I think we need a consistent error return API for functions in >> HostInfo/HostThread to say "not available on this platform" and generic >> code can use a different approach. >> > Not all methods even return Error. And the cost of making every single > method return Error is even higher, because nobody is ever going to check > for this return value. And what do you do when you do get the return > value? Call exit()? If the compiler catches this for you it doesn't have > to be a runtime decision. > > Again, the number of places where this is going to happen are small. As > Jim said, most generic LLDB code isn't and shouldn't be calling into Host. > You can search through my HostInfo patch, for example, and find that I > converted maybe 100 pre-processor definitions into about 2 or 3. I don't > think this problem of #ifdef'ing will be as bad as you think it will be, > and certainly it's better than before? > > HostInfo has another issue, btw, which is that it is a static class. So > there's really no other way. There's no instance of it, just as there was > no instance of Host. I could have gone and added methods for Windows jobs, > SEH, and plenty of other highly windows specific concepts to Host.h and > grew the class even more than it already was, and then stubbed out all the > methods on every other platform, but is that really what you want? More > likely to break the build, increases code size, increases build time, all > for code that is never going to run on the platform. And since, by > definition, the code should never run on the platform, why not find out at > compile time if some code path calls it? > > The idea of "I'll just implement this method for my platform and stub it > out for everyone else" just shifts the burden from a compile time burden to > a run-time burden, which is almost always a worse proposition to have to > deal with. In the end, the "public" interface of any given Host class is > likely to stabilize and change very infrequently. The platform specific > bits, on the other hand, are likely to evolve quite regularly. A design > where people can work on the platform specific bits in isolation from every > other platform without affecting anyone else is a huge win across the board. >
_______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
