search

theRockLiu / googletest

Automatically exported from code.google.com/p/googletest
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

gtest-port.h use of #if prevents compilation with -Wundef #240

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
What steps will reproduce the problem?
1. Build gtest
2. Write a test
3. Compile with -Wundef passed to g++

What is the expected output? What do you see instead?
I would expect a clean compile with no warnings emitted.  But the use of
#if with a mixture of macros, some defined and some that are not generates
a large number of warnings.

What version of the product are you using? On what operating system?
Version: Revision 357
OS: Linux (Ubuntu 9.10)

Please provide any additional information below, such as a code snippet.
I have attached a patch to gtest-port.h that fixes this issue by adding
definitions for all of the GTEST_OS_* macros.  They default to 0 and are
redefined to 1 as needed.

Original issue reported on code.google.com by robot...@chromium.org on 28 Dec 2009 at 12:55

Attachments:

GoogleCodeExporter commented 9 years ago 
Thanks for contributing, robotboy.

Unfortunately this is a breaking change.  Before, people could write either

  #if GTEST_OS_WINDOWS

or

  #ifdef GTEST_OS_WINDOWS

and they mean the same thing.  After this change, the meaning of the
latter is silently changed.  If we were to make this change, we have
to first fix all existing users (at lease those inside Google, who'll
be directly affected), which can be a lot of work.

Also, the new definitions are more error-prone.  After the change, if
someone accidentally writes #ifdef GTEST_OS_WINDOWS instead of #if
GTEST_OS_WINDOWS, the code may still compile, but not in the way he
expects.  I like that the old definitions let you use both forms
interchangeably.

Ideally gtest should compile without any warning.  In this case,
fixing this warning is expensive and leads to new problems.  -Wundef
is not part of -Wall or -Wextra, so I'm fine with not being clean
here.

BTW, I don't like how GCC's -Wundef works.  Treating undefined macros
as 0 is a standard technique explicitly allowed by the C/C++ standard.
GCC should complain about a macro FOO in a #if context *only if*
"#define FOO ..." doesn't appear in the source code (even if it's
#if-ed out).

(Note that the GTEST_HAS_* macros have the same problem as GTEST_OS_*,
and we should keep their behaviors consistent.)

Original comment by w...@google.com on 28 Dec 2009 at 7:03

GoogleCodeExporter commented 9 years ago 
    You are absolutely right.  I failed to consider the breaks this would cause in 
existing code.  I have attached a patch that you may find more acceptable.  
This patch 
adds GTEST_DEFINE_OS_MACROS which the user can define to enable the ability to 
build 
with -Wundef.  If this macro is not defined then the GTEST_OS_* macros behave 
as they 
do currently.

    I would argue with your second point however.  gtest-port.h is fairly inconsistent 
(but documented) with it's use of undefined and 0 to mean the lack of a 
feature.  All 
of the GTEST_HAS_CLONE type macros will be defined to 1 or 0 by the time the 
user can 
use them in a source file.  I realize these are more about telling gtest-port 
how to 
behave.  But GTEST_IS_THREADSAFE is also defined to 0 instead of left undefined 
to 
indicate that GTEST is not threadsafe.  So people already have to be very 
careful when 
they try to test these macros.  Secondly, the use of #ifdef can introduce bugs 
by 
masking spelling errors in the macro name.  So #ifdef GTEST_OS_WINDOWS_MOBILE 
and 
#ifdef GTEST_OS_WINODWS_MOBILE would also both compile but not in the way the 
user 
expects.

    Consistency of behavior is good.  That said, my patch doesn't attempt to change the 
behavior of the GTEST_HAS_ macros that are left undefined for two reasons.  
One, there 
are many more uses of them and the semantics are less obvious.  And two, they 
all seem 
to be used in a way that is consistent with the use of -Wundef, so they are not 
a 
problem for me when compiling my code with -Wundef.

    Lastly, I could also go through and modify all instances of #if GTEST_OS_* and 
similar to be #if defined(GTEST_OS_*) if that solution sounded better to you.  
I did 
most of that work already in a separate branch and it works, it just requires 
modifying 
many more files.  My current patch seemed less intrusive.

    Thanks,
        Anton

Original comment by robot...@chromium.org on 28 Dec 2009 at 8:56

Attachments: