Macro redefinition problem (SUCCEED/FAIL)

[GoogleCodeExporter]

Many existing software libraries define their own SUCCEED and FAIL macros.
The following links show some of them:

http://www.google.com/codesearch?hl=en&lr=&q=%22%23define+SUCCESS%22&sbtn=Search
http://www.google.com/codesearch?hl=en&lr=&q=%22%23define+FAIL%22&sbtn=Search

If googletest is used with such library, compiler complains about the macro
redefinition. I got the following errors while compiling with HDF4 library.

In file included from gtest_dimmap.cc:3:
/scr/eseo2/local/hdf-4.2.5/include/hdf.h:93:1: warning: "SUCCEED" redefined
In file included from gtest_dimmap.cc:1:
/scr/eseo2/local/gtest-1.4.0/include/gtest/gtest.h:1582:1: warning: this is
the location of the previous definition
In file included from gtest_dimmap.cc:3:
/scr/eseo2/local/hdf-4.2.5/include/hdf.h:94:1: warning: "FAIL" redefined
In file included from gtest_dimmap.cc:1:
/scr/eseo2/local/gtest-1.4.0/include/gtest/gtest.h:1580:1: warning: this is
the location of the previous definition

This problem happens with other macros as well, but SUCCEED and FAIL are
much general term and other and cause much more problem.

Therefore, what about changing them to inline functions to put them into
the testing namespace as follow?

// gtest.h
1578 #if 0
1579 // Generates a fatal failure with a generic message.
1580 #define FAIL() GTEST_FATAL_FAILURE_("Failed")
1581 // Generates a success with a generic message.
1582 #define SUCCEED() GTEST_SUCCESS_("Succeeded")
1583
1584 #else
1585 inline void FAIL()
1586 {
1587     GTEST_FATAL_FAILURE_("Failed");
1588 }
1589
1590 inline void SUCCEED()
1591 {
1592     GTEST_SUCCESS_("Succeeded");
1593 }
1594 #endif

Original issue reported on code.google.com by eun...@gmail.com on 2 Apr 2010 at 8:57

[GoogleCodeExporter]

Unfortunately, the suggested fix will will break streaming into SUCCEED() and 
FAIL() macros (e.g., FAIL() << 
"error  details").

It looks like we need a more generic solutions, since this is not the only 
instance of macro name collision (see 
issue http://code.google.com/p/googletest/issues/detail?id=162). We need an 
approach that deals with all 
generically named macros. One solution would be give such generic macros 
GTEST_-prefixed aliases and 
suppress definition of generic names if a symbol like 
GTEST_NO_GENERIC_MACRO_NAMES is defined.

We may not have resources to implement this in the near term, do you care to 
contribute a patch?

Original comment by vladlosev on 5 Apr 2010 at 8:50

• Changed state: Accepted
• Added labels: Component-Logic, OpSys-All, Priority-Low, Type-Enhancement, Usability
• Removed labels: Priority-Medium, Type-Defect

[GoogleCodeExporter]

This is a problem with C++ in general, and not specific to gtest or any other 
C++ 
package.

It doesn't work to define FAIL() etc as inline functions, as we need to be able 
to 
return from the current function.

A control macro like GTEST_NO_GENERIC_MACRO_NAMES makes everything more 
complex.  
Also the result is too verbose to use (do you really want to type GTEST_TEST, 
GTEST_ASSERT_TRUE, etc, instead of TEST, ASSERT_TEST, etc just so that you can 
use 
GTEST_FAIL?).

I think the right approach to deal with this is for a user running into macro 
name 
clashes to rename the clashing names locally.  Use sed to change all FAIL 
(match by 
word) in gtest source to GTEST_FAIL, for example.  It's unpleasant but such is 
life 
when working with C++.

Original comment by w...@google.com on 5 Apr 2010 at 3:33

• Changed state: WontFix

[GoogleCodeExporter]

The reason why I think inlining will work is that FAIL() is under testing() 
namespace. 
Macro does not respect namespace, but inline function does.
Since it is in the namespace, users can avoid typing more characters by using 
"using 
namespace".

Original comment by eun...@gmail.com on 5 Apr 2010 at 4:09

[GoogleCodeExporter]

As said, an inline function can not return from its caller, which FAIL needs to 
do.  
Try to implement it as an inline function and run gtest's tests, then you'll 
see.

Original comment by w...@google.com on 5 Apr 2010 at 4:12

[GoogleCodeExporter]

wan:
I got your point. You meant that FAIL() macro includes "return". You're right. 
However, 
in that case, you can still use exception to return to the original caller.

Original comment by eun...@gmail.com on 5 Apr 2010 at 8:47

[GoogleCodeExporter]

gtest needs to work when exceptions are disabled, so FAIL cannot throw.

Even if we could define FAIL() as a function, it doesn't solve your problem.  As
packages who define FAIL() as a macro will still clash with it.

#include <gtest/gtest.h>
#include <some/package.h>  // This defines macro FAIL.

...
FAIL();  // This FAIL will be treated as the macro defined in some/package.h.

Original comment by w...@google.com on 5 Apr 2010 at 9:44

[GoogleCodeExporter]

wan:
I'm not proposing a magic solution which solves all problems without any cost. 
In my 
opinion, it is too harsh and unkind to enforce users to modify their code 
(which 
could have been widely used for many years) to use gtest. If the code is a part 
of 
some library, renaming all FAIL may break other programs. I think this is an 
important problem which needs to be solved even with some cost.

One way to solve this is, as you said, renaming macros with general names. If 
GTEST_TEST and GTEST_ASSERT_TRUE are too long, you may use GTEST and 
GASSERT_TRUE. I 
think that is enough. Of course, current testing programs which use gtest needs 
to be 
modified appropriately.

Another way is, as I proposed, using namespace, inline function, and exception. 
For 
this, of course, testing programs also need to be changed. In many cases, 
"using 
namespace" can make this easy. In your example, testing::Fail() needs to be 
used 
instead of TEST().

Original comment by eun...@gmail.com on 5 Apr 2010 at 10:05

[GoogleCodeExporter]

Now I feel that control macro is the best option which preserves backward 
compatibility. If GTEST_NO_GENERIC_MACRO_NAMES is set, GTEST_TEST, 
GTEST_ASSERT_TRUE,... are  used. Otherwise, original macros are used. :-)

You may discourage the use of GTEST_NO_GENERIC_MACRO_NAMES to the gtest users, 
but it 
is good to have a way to make gtest compatible with other legacy code.

Original comment by eun...@gmail.com on 5 Apr 2010 at 10:14

[GoogleCodeExporter]

I didn't propose to ask the users to modify their own code.  What I said was to 
let
the users modify their copy of gtest.

A control macro makes everything more complex.  It's a big price to pay in 
terms of
mantenability and simplicity.  It's not a good user experience either, as it 
forces
you to use the more verbose names everywhere if you have a single clashing 
macro.

I still believe the best solution is for the user to rename gtest's definitions 
in
his own copy of gtest.

If you still have comments or concerns, you are welcome to post them on the
googletestframework@googlegroups.com mailing list, where more people can chime 
in. 
The issue tracker is not a good forum for discussions.  Thanks.

Original comment by w...@google.com on 5 Apr 2010 at 10:21

[GoogleCodeExporter]

Kenton Varda suggested to use a separate control macro for each macro that 
clashes
(so we'll be enabling FAIL and SUCCESS separately).  I like it better as the 
user
isn't forced to use GTEST_* everywhere when he runs into a single clashing 
macro.

// The user writes this if FAIL clashes.
#define GTEST_DONT_DEFINE_FAIL 1

// gtest.h
#define GTEST_FAIL() ...

#if !GTEST_DONT_DEFINE_FAIL
#define FAIL() GTEST_FAIL()
#endif

Original comment by w...@google.com on 7 Apr 2010 at 9:01

• Changed state: Accepted

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

According to Kenton's suggestion, I made the following changes and confirmed 
that it 
does not affect current googletest users.

It seems that the most generic terms are SUCCEED, FAIL and TEST, so I added 
control 
macros for them only.

Index: include/gtest/gtest.h
===================================================================
--- include/gtest/gtest.h       (revision 412)
+++ include/gtest/gtest.h       (working copy)
@@ -1651,10 +1651,16 @@
 #define ADD_FAILURE() GTEST_NONFATAL_FAILURE_("Failed")

 // Generates a fatal failure with a generic message.
-#define FAIL() GTEST_FATAL_FAILURE_("Failed")
+#define GTEST_FAIL() GTEST_FATAL_FAILURE_("Failed")
+#ifndef GTEST_DONOT_DEFINE_FAIL
+#define FAIL GTEST_FAIL
+#endif

 // Generates a success with a generic message.
-#define SUCCEED() GTEST_SUCCESS_("Succeeded")
+#define GTEST_SUCCEED() GTEST_SUCCESS_("Succeeded")
+#ifndef GTEST_DONOT_DEFINE_SUCCEED
+#define SUCCEED GTEST_SUCCEED
+#endif

 // Macros for testing exceptions.
 //
@@ -1986,9 +1992,12 @@
 // code.  GetTestTypeId() is guaranteed to always return the same
 // value, as it always calls GetTypeId<>() from the Google Test
 // framework.
-#define TEST(test_case_name, test_name)\
+#define GTEST_TEST(test_case_name, test_name)\
   GTEST_TEST_(test_case_name, test_name, \
               ::testing::Test, ::testing::internal::GetTestTypeId())
+#ifndef GTEST_DONOT_DEFINE_TEST
+#define TEST GTEST_TEST
+#endif

 // Defines a test that uses a test fixture.

Original comment by eun...@gmail.com on 12 Apr 2010 at 10:08

[GoogleCodeExporter]

Bump up the priority as there's a patch submitted by the user.

Original comment by w...@google.com on 12 Apr 2010 at 10:12

• Added labels: Priority-High
• Removed labels: Priority-Low

[GoogleCodeExporter]

Fixed in trunk r414.

Original comment by w...@google.com on 13 Apr 2010 at 4:41

• Changed state: Fixed

[GoogleCodeExporter]

Issue 352 has been merged into this issue.

Original comment by vladlosev on 13 Jan 2011 at 9:52