On Thu, Dec 10, 2009 at 10:45 AM, Jonathan Dixon <[email protected]> wrote:
> > > 2009/12/10 Brett Wilson <[email protected]> > > On Wed, Dec 9, 2009 at 4:24 PM, John Abd-El-Malek <[email protected]> >> wrote: >> > btw I searched the code, almost all the instances are in code from >> different >> > repositories, like v8, gtest, gmock. I counted only 17 instances in >> > Chrome's code. >> >> >> Most of the Chrome NOLINTs look like the're around ifdefs, where the >> ifdef code sometimes mean that a comma or a semicolon goes on the line >> after the ifdef. We should be working to remove these anyway since the >> ifdefs are super ugly, and I'm not sure the NOLINT comment actually >> makes them worse. Some of these may not be practical or desirable to >> remove, though. >> >> > This is exactly the case I came across recently (which maybe what inspired > John to start this thread.) > > In essence: > > return DoWork(&foo) > #if defined(OS_POSIX) > && DoWork(&posix_specific) > #endif > ; // <-- Lint complains about this guy > > We converged on NOLINT as the solution, but I admit my ingrained instinct > is to pull out the #if altogether, and try to make both calls to DoWork() be > acceptable to call on all platforms, or at the very least replace the second > with a wrapper its body with a null implementation on !OS_POSIX. > > Do we have agreed guidelines on when to use #if for portability, or which > patterns to prefer to it? > >From the code I've seen, the preference is if only one platform needs a function to be called, then we only call it using an ifdef instead of defining that empty function in all other platforms. Things like PlatformInit, PlatformDestroy are an exeception since it makes sense that you want to give each platform the capability for that. But if you look at places like PluginService or PluginProcessHost, there are things that only make sense to call on one platform. -- Chromium Developers mailing list: [email protected] View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev
