base: Disallow mixing NDEBUG settings in logging.{h,cc}.

base: Disallow mixing NDEBUG settings in logging.{h,cc}.

I am concerned about the possibility that other users of the
files in Chrome's base/ directory (e.g. Chrome OS) may build
a library from these files with NDEBUG defined and then link
it into a binary that includes the headers without defining
NDEBUG, or vice versa.

This change mangles InitLogging()'s name in logging.h based
on whether NDEBUG is defined so that linking will fail if
there's a mismatch (Evan provided useful advice here).

BUG=chromium-os:1124
TEST=built Debug and Release Chrome, built all Chrome OS packages, and then changed a Chrome OS package that links against libbase.a to define NDEBUG and checked that it failed to link

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=57098

[brettw, 2010-08-20 22:56:59]

I'm scared of this define. Is there a way we can do this for some more unique
name? Currently everything is inside a logging namespace to help with this
problem, and I'm forever running into problems with Windows redefining standard
names to something else which causes almost impossible to find problems.

InitLogging seems like something that could be defined elsewhere. In fact, it
is, in webkit_support.cc there is a local function called InitLogging.

[Daniel Erat, 2010-08-20 23:11:38]

Good point.  I'll try to come up with something better.

On Fri, Aug 20, 2010 at 3:56 PM,  <brettw@chromium.org> wrote:
> I'm scared of this define. Is there a way we can do this for some more
> unique
> name? Currently everything is inside a logging namespace to help with this
> problem, and I'm forever running into problems with Windows redefining
> standard
> names to something else which causes almost impossible to find problems.
>
> InitLogging seems like something that could be defined elsewhere. In fact,
> it
> is, in webkit_support.cc there is a local function called InitLogging.
>
> http://codereview.chromium.org/3106032/show
>

[Daniel Erat, 2010-08-21 00:39:04]

I've added a more-specifically-named intermediate function and changed the
#defines to rename it instead.

I'm still not completely happy with this approach; it seems ugly to repeat the
function signatures several times, and there's no guarantee that users of this
file will even call InitLogging() (although they ought to).  Do you have any
suggestions?

On 2010/08/20 23:11:38, Daniel Erat wrote:
> Good point.  I'll try to come up with something better.
> 
> On Fri, Aug 20, 2010 at 3:56 PM,  <mailto:brettw@chromium.org> wrote:
> > I'm scared of this define. Is there a way we can do this for some more
> > unique
> > name? Currently everything is inside a logging namespace to help with this
> > problem, and I'm forever running into problems with Windows redefining
> > standard
> > names to something else which causes almost impossible to find problems.
> >
> > InitLogging seems like something that could be defined elsewhere. In fact,
> > it
> > is, in webkit_support.cc there is a local function called InitLogging.
> >
> > http://codereview.chromium.org/3106032/show
> >
>

[brettw, 2010-08-21 02:05:44]

This should at least be "BaseInitLogging" since base doesn't
necessarily mean Chrome. I can't think of a better approach.

Brett

On Fri, Aug 20, 2010 at 3:50 PM,  <derat@chromium.org> wrote:
> Reviewers: brettw,
>
> Description:
> base: Disallow mixing NDEBUG settings in logging.{h,cc}.
>
> I am concerned about the possibility that other users of the
> files in Chrome's base/ directory (e.g. Chrome OS) may build
> a library from these files with NDEBUG defined and then link
> it into a binary that includes the headers without defining
> NDEBUG, or vice versa.
>
> This change mangles InitLogging()'s name in logging.h based
> on whether NDEBUG is defined so that linking will fail if
> there's a mismatch (Evan provided useful advice here).
>
> BUG=chromium-os:1124
> TEST=built Debug and Release Chrome, built all Chrome OS packages, and then
> changed a Chrome OS package that links against libbase.a to define NDEBUG
> and
> checked that it failed to link
>
> Please review this at http://codereview.chromium.org/3106032/show
>
> SVN Base: http://src.chromium.org/git/chromium.git
>
> Affected files:
>  M base/logging.h
>
>
> Index: base/logging.h
> diff --git a/base/logging.h b/base/logging.h
> index
>
69c67f487b9c7b213dcfa273172c7cfc5f3de18a..2f6ac7dc4dfa967d1e8cdf39b635a52670ff0c2c
> 100644
> --- a/base/logging.h
> +++ b/base/logging.h
> @@ -126,6 +126,16 @@ enum LogLockingState { LOCK_LOG_FILE,
> DONT_LOCK_LOG_FILE };
>  // Defaults to APPEND_TO_OLD_LOG_FILE.
>  enum OldFileDeletionState { DELETE_OLD_LOG_FILE, APPEND_TO_OLD_LOG_FILE };
>
> +// Define different names for the InitLogging() function depending on
> whether
> +// NDEBUG is defined or not so that we'll fail to link if someone tries to
> +// compile logging.cc with NDEBUG but includes logging.h without defining
> it, or
> +// vice versa.
> +#if NDEBUG
> +#define InitLogging InitLogging_built_with_NDEBUG
> +#else
> +#define InitLogging InitLogging_built_without_NDEBUG
> +#endif
> +
>  // Sets the log file name and other global logging state. Calling this
> function
>  // is recommended, and is normally done at the beginning of application
> init.
>  // If you don't call it, all the flags will be initialized to their default
>
>
>

[Daniel Erat, 2010-08-23 16:46:29]

Thanks, another look?  I also moved a typedef from the .cc file to the header so
we don't need multiple declarations of the functions.

[brettw, 2010-08-23 16:49:08]

LGTM