In general I look suspiciously at all uses of #ifdef SOME_HOST_THING. In some cases, like ConnectionFileDescriptor where this is really a host functionality, but there's enough common code that it wasn't worthwhile to try to tease out into host specific subclasses, it's kinda okay to do it this way. More of a perfect is the enemy of the good type thing.
But for instance the fact that ProcessLaunchInfo has a method that #ifdef'ed LLDB_DISABLE_POSIX means somebody was taking a shortcut. Again, time and the need for forward progress being what they are, maybe that's an okay tradeoff. But you should probably mentally give yourself a demerit for doing it at least... Jim > On Jul 3, 2014, at 1:12 PM, Zachary Turner <[email protected]> wrote: > > Even simple typedefs already imply a host dependency. Currently platform > specific typedefs are not brought in through Host/Config.h, but they could be > for example. But I guess the point is that if you're compiling for a > particular host, you're already married to it. > > I guess I only find this an issue because the codebase or style guide doesn't > seem to discourage using #ifdefs for platform-specific logic. So say I'm > editing some file, and I see the use of LLDB_DISABLE_POSIX. I then go use > that same define in another file because I need it, and it's not defined > because Config.h isn't included. Including it is obviously an easy fix, but > it just strikes me as inconsistent that it would be defined in some places > and not others. What if someone uses it in a header file? (Note that it's > already used in a header file, btw. ProcessLaunchInfo.h). > > > On Thu, Jul 3, 2014 at 12:35 PM, <[email protected]> wrote: > This seems a little backward to me. We should be encouraging all the generic > code in lldb to have no host dependencies unless they are absolutely > necessary, shouldn't we? Making all .cpp files include a host specific > header file seems going counter to this aim. > > Jim > > > > On Jul 2, 2014, at 10:04 PM, Zachary Turner <[email protected]> wrote: > > > > It would be useful if there were a header file that were always included in > > every CPP file before any other header file, that would initialize any > > platform-specific defines and whatnot. We already have a header file that > > seems to serve exactly this purpose, but it just isn't always included. > > It's lldb/Host/Config.h > > > > Doing a single pass over every file and manually including this would be a > > lot of work, but can we make it a policy going forward that a) this file > > will always be included first, in every newly created cpp file, and b) When > > touching files, try to remember to add this include at the top? > > _______________________________________________ > > lldb-dev mailing list > > [email protected] > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev > > _______________________________________________ lldb-dev mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
