|
|
|
This patch makes it possible to build on currently unsupported platforms and compilers, including:
AIX - GCC
HP-UX - HP-GCC
Solaris - GCC
z/OS - IBM XL C/C++
It also fixes the Visual Studio project, which was missing to reference a source file.
Please note that the build system has not been touched, as on most of these platforms neither of the currently used build systems is anything like standard.
It might be questionable if _POSIX_PATH_MAX is safe to be used if PATH_MAX is not defined. One of these platforms is z/OS.
Patch Set 1 #Patch Set 2 : Portability patch taking comments into consideration - vcproj changes moved to Issue 7707 #Patch Set 3 : fixing comment and misspelt macro #Patch Set 4 : Patch as proposed by Zhanyong via mailinglist #Patch Set 5 : Patch as proposed by Zhanyong via mailinglist (fixed line endings) #Patch Set 6 : Fixes as suggested by Zhanyong #
Total messages: 20
|
Hobbes
|
17 years, 2 months ago (2008年10月23日 12:16:21 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||
I've made an initial pass through this patch, thought I'd go ahead and send it out to you. http://codereview.appspot.com/7855/diff/1/3 File include/gtest/internal/gtest-port.h (right): http://codereview.appspot.com/7855/diff/1/3#newcode559 Line 559: // Some Compilers do not have tr1::type_traits, so we define our tr1::type_traits and the issue with template instatiation don't seem to be well described by "NEED_TEMPLATE_WORKAROUND". Is it just coincidence that both of these compilers necessitate the same set of workarounds? If so, I'd actually rather list both define guards at each place lest one of the workarounds cease to be necessary on one of these tool chains.u If it isn't coincidence, I'd prefer the symbol indicate what point of commonality is being handled (and a comment at the definition of the symbol to explain this). For example, if these tool chains are related, or share components from some vendor, make it GTEST_SOME_TOOL_CHAIN_VENDOR, and explain what the implications are in the comment. http://codereview.appspot.com/7855/diff/1/6 File msvc/gtest.vcproj (right): http://codereview.appspot.com/7855/diff/1/6#newcode133 Line 133: RelativePath="..\src\gtest-test-part.cc"> I think this should be a separate patch as it should have been added to the visual studio project when it was first committed. http://codereview.appspot.com/7855/diff/1/4 File src/gtest-filepath.cc (right): http://codereview.appspot.com/7855/diff/1/4#newcode56 Line 56: #define PATH_MAX _POSIX_PATH_MAX Can we group this with the <limits.h> that "should" provide PATH_MAX? (I agree it is *highly* frustrating to not have a standard to fall back on for PATH_MAX.) http://codereview.appspot.com/7855/diff/1/5 File src/gtest.cc (right): http://codereview.appspot.com/7855/diff/1/5#newcode94 Line 94: #elif defined(GTEST_OS_ZOS) What about combining this with the OS_LINUX block above, and enclosing the 3 or 4 headers that are linux-specific in an OS_LINUX-only block? It would reduce the repetition. We might also find we don't need to include some of the ones we're currently including. That said, you are following the existing pattern here, I just don't like that pattern. ;]
http://codereview.appspot.com/7855/diff/1/2 File include/gtest/internal/gtest-internal.h (right): http://codereview.appspot.com/7855/diff/1/2#newcode154 Line 154: // Passing non-POD classes through ellipsis (...) crashes some compilers. The comments about ARM and Nokia Symbian are valuable information, and shouldn't be erased. You can for example say "crashes some compilers, including the ARM compiler". http://codereview.appspot.com/7855/diff/1/2#newcode201 Line 201: // These are needed as some Compilers cannot decide between The same. http://codereview.appspot.com/7855/diff/1/3 File include/gtest/internal/gtest-port.h (right): http://codereview.appspot.com/7855/diff/1/3#newcode166 Line 166: #define GTEST_OS_ZOS Wow, I never thought someone would use Google Test there. :-) http://codereview.appspot.com/7855/diff/1/3#newcode210 Line 210: // At least some versions of cygwin doesn't support ::std::wstring. Update the comment to include Solaris. http://codereview.appspot.com/7855/diff/1/3#newcode554 Line 554: #define GTEST_NEED_TEMPLATE_WORKAROUND 1 I agree with Chandler's comments. http://codereview.appspot.com/7855/diff/1/3#newcode559 Line 559: // Some Compilers do not have tr1::type_traits, so we define our Put "Symbian" back. http://codereview.appspot.com/7855/diff/1/4 File src/gtest-filepath.cc (right): http://codereview.appspot.com/7855/diff/1/4#newcode56 Line 56: #define PATH_MAX _POSIX_PATH_MAX We shouldn't try to define a standard macro, which could lead to subtle problems. Instead, I would do this in gtest-port.h: #ifdef PATH_MAX #define GTEST_PATH_MAX_ PATH_MAX #else #define GTEST_PATH_MAX_ _POSIX_PATH_MAX #endif // PATH_MAX and use GTEST_PATH_MAX_ through out. Actually, is _POSIX_PATH_MAX guaranteed to be defined when PATH_MAX is not? http://codereview.appspot.com/7855/diff/1/5 File src/gtest.cc (right): http://codereview.appspot.com/7855/diff/1/5#newcode94 Line 94: #elif defined(GTEST_OS_ZOS) On 2008年10月24日 22:41:42, chandlerc wrote: > What about combining this with the OS_LINUX block above, and enclosing the 3 or > 4 headers that are linux-specific in an OS_LINUX-only block? It would reduce the > repetition. We might also find we don't need to include some of the ones we're > currently including. > > That said, you are following the existing pattern here, I just don't like that > pattern. ;] I like this idea.
To respond to the email comment by Rainer (JSYK, you can respond to in-line comments within the code review tool): How about GTEST_OS_POSIX? If necessary we can qualify it as GTEST_OS_POSIX_SIMILAR. http://codereview.appspot.com/7855/diff/1/4 File src/gtest-filepath.cc (right): http://codereview.appspot.com/7855/diff/1/4#newcode56 Line 56: #define PATH_MAX _POSIX_PATH_MAX On 2008年10月29日 22:22:25, Zhanyong wrote: > We shouldn't try to define a standard macro, which could lead to subtle > problems. > > Instead, I would do this in gtest-port.h: > > #ifdef PATH_MAX > #define GTEST_PATH_MAX_ PATH_MAX > #else > #define GTEST_PATH_MAX_ _POSIX_PATH_MAX > #endif // PATH_MAX > > and use GTEST_PATH_MAX_ through out. Agreed. > > Actually, is _POSIX_PATH_MAX guaranteed to be defined when PATH_MAX is not? This is sticky. POSIX allows you an implementation to omit the definition of PATH_MAX. It does say that limits.h *must* provide _POSIX_PATH_MAX, but this is defined to be 256, and a minimum value for PATH_MAX. One thing to consider is that this is a *very* limiting max. ;] We can also check for _XOPEN_PATH_MAX. If its defined, POSIX also guarantees it as a minimum as well. Last but not least, we can jump through all of the POSIX hoops and if PATH_MAX isn't defined by limits.h, we can get it by calling the routine: long pathconf("/some/path/name", _PC_PATH_MAX); This allows some implementations to let PATH_MAX vary from path to path. The first argument must always be a directory, and the return is the maximum path length for a relative path given the argument as a working directory. Overkill anyone? ;] If _POSIX_PATH_MAX is enough, I'd just do a tiered fall off: #ifdef PATH_MAX #define GTEST_PATH_MAX_ PATH_MAX #elif defined(_XOPEN_PATH_MAX) #define GTEST_PATH_MAX_ _XOPEN_PATH_MAX #else #define GTEST_PATH_MAX_ _POSIX_PATH_MAX #endif // PATH_MAX
http://codereview.appspot.com/7855/diff/1/4 File src/gtest-filepath.cc (right): http://codereview.appspot.com/7855/diff/1/4#newcode56 Line 56: #define PATH_MAX _POSIX_PATH_MAX On 2008年10月30日 06:49:56, chandlerc wrote: > On 2008年10月29日 22:22:25, Zhanyong wrote: > > We shouldn't try to define a standard macro, which could lead to subtle > > problems. > > > > Instead, I would do this in gtest-port.h: > > > > #ifdef PATH_MAX > > #define GTEST_PATH_MAX_ PATH_MAX > > #else > > #define GTEST_PATH_MAX_ _POSIX_PATH_MAX > > #endif // PATH_MAX > > > > and use GTEST_PATH_MAX_ through out. > > Agreed. > > > > > Actually, is _POSIX_PATH_MAX guaranteed to be defined when PATH_MAX is not? > > This is sticky. POSIX allows you an implementation to omit the definition of > PATH_MAX. It does say that limits.h *must* provide _POSIX_PATH_MAX, but this is > defined to be 256, and a minimum value for PATH_MAX. One thing to consider is > that this is a *very* limiting max. ;] > > We can also check for _XOPEN_PATH_MAX. If its defined, POSIX also guarantees it > as a minimum as well. > > Last but not least, we can jump through all of the POSIX hoops and if PATH_MAX > isn't defined by limits.h, we can get it by calling the routine: > > long pathconf("/some/path/name", _PC_PATH_MAX); > > This allows some implementations to let PATH_MAX vary from path to path. The > first argument must always be a directory, and the return is the maximum path > length for a relative path given the argument as a working directory. Overkill > anyone? ;] > > If _POSIX_PATH_MAX is enough, I'd just do a tiered fall off: This should be good enough. > > #ifdef PATH_MAX > #define GTEST_PATH_MAX_ PATH_MAX > #elif defined(_XOPEN_PATH_MAX) > #define GTEST_PATH_MAX_ _XOPEN_PATH_MAX > #else > #define GTEST_PATH_MAX_ _POSIX_PATH_MAX > #endif // PATH_MAX Perhaps we should provide a hard-coded default value if none of the standard macros is defined.
http://codereview.appspot.com/7855/diff/1/4 File src/gtest-filepath.cc (right): http://codereview.appspot.com/7855/diff/1/4#newcode56 Line 56: #define PATH_MAX _POSIX_PATH_MAX On 2008年10月30日 06:55:08, Zhanyong wrote: > On 2008年10月30日 06:49:56, chandlerc wrote: > > On 2008年10月29日 22:22:25, Zhanyong wrote: > > > We shouldn't try to define a standard macro, which could lead to subtle > > > problems. > > > > > > Instead, I would do this in gtest-port.h: > > > > > > #ifdef PATH_MAX > > > #define GTEST_PATH_MAX_ PATH_MAX > > > #else > > > #define GTEST_PATH_MAX_ _POSIX_PATH_MAX > > > #endif // PATH_MAX > > > > > > and use GTEST_PATH_MAX_ through out. > > > > Agreed. > > > > > > > > Actually, is _POSIX_PATH_MAX guaranteed to be defined when PATH_MAX is not? > > > > This is sticky. POSIX allows you an implementation to omit the definition of > > PATH_MAX. It does say that limits.h *must* provide _POSIX_PATH_MAX, but this > is > > defined to be 256, and a minimum value for PATH_MAX. One thing to consider is > > that this is a *very* limiting max. ;] > > > > We can also check for _XOPEN_PATH_MAX. If its defined, POSIX also guarantees > it > > as a minimum as well. > > > > Last but not least, we can jump through all of the POSIX hoops and if PATH_MAX > > isn't defined by limits.h, we can get it by calling the routine: > > > > long pathconf("/some/path/name", _PC_PATH_MAX); > > > > This allows some implementations to let PATH_MAX vary from path to path. The > > first argument must always be a directory, and the return is the maximum path > > length for a relative path given the argument as a working directory. Overkill > > anyone? ;] > > > > If _POSIX_PATH_MAX is enough, I'd just do a tiered fall off: > > This should be good enough. > > > > > #ifdef PATH_MAX > > #define GTEST_PATH_MAX_ PATH_MAX > > #elif defined(_XOPEN_PATH_MAX) > > #define GTEST_PATH_MAX_ _XOPEN_PATH_MAX > > #else > > #define GTEST_PATH_MAX_ _POSIX_PATH_MAX > > #endif // PATH_MAX > > Perhaps we should provide a hard-coded default value if none of the standard > macros is defined. Well, <limits.h> and <unistd.h> are POSIX headers, and _POSIX_PATH_MAX is absolutely required for POSIX. It's value is defined by the standard as 256, so you could use that, but I don't see a win. Special case the platform if they are sufficiently broken to diverge from POSIX despite supporting its headers. The plan was to move all this up to the <limits.h> include, where it'll be grouped by GTEST_OS_POSIX. We can then provide more intelligent defines of PATH_MAX for the various windows and non-POSIX OSes.
Portability patch taking comments into consideration - vcproj changes moved to Issue 7707
http://codereview.appspot.com/7855/diff/15/210 File include/gtest/internal/gtest-port.h (right): http://codereview.appspot.com/7855/diff/15/210#newcode560 Line 560: #define GTEST_COMPILER_NEEDS_POINTER_TEMPLETE_SELECTION_HELP 1 i guess this macro is explaining the issue somewhat better. this is taken from http://codereview.appspot.com/download/issue6051_212.diff http://codereview.appspot.com/7855/diff/15/211 File src/gtest-filepath.cc (right): http://codereview.appspot.com/7855/diff/15/211#newcode55 Line 55: #define getcwd _getcwd this is in line with how it's handled in src/gtest.cc around line 123 http://codereview.appspot.com/7855/diff/15/212 File src/gtest.cc (right): http://codereview.appspot.com/7855/diff/15/212#newcode108 Line 108: #if defined(GTEST_OS_ZOS) actually the separate branch for z/OS it isn't really needed as the 'others' section pretty much covers it. only exception needed is, that on z/OS we need to include strings.h for strcasecmp http://codereview.appspot.com/7855/diff/15/212#newcode2453 Line 2453: #if defined(_WIN32_WCE) || defined(GTEST_OS_SYMBIAN) || defined(GTEST_OS_ZOS) ANSI escape sequences didn't work for me on z/OS. Either it's because of the TERM type or because of EBCDIC. My guess is the latter.
http://codereview.appspot.com/7855/diff/15/209 File include/gtest/internal/gtest-internal.h (right): http://codereview.appspot.com/7855/diff/15/209#newcode242 Line 242: #endif // GTEST_OS_SYMBIAN Small nit: GTEST_COMPILER_NEEDS_POINTER_TEMPLETE_SELECTION_HELP
http://codereview.appspot.com/7855/diff/15/209 File include/gtest/internal/gtest-internal.h (right): http://codereview.appspot.com/7855/diff/15/209#newcode242 Line 242: #endif // GTEST_OS_SYMBIAN On 2008年10月30日 15:50:02, Vlad wrote: > Small nit: GTEST_COMPILER_NEEDS_POINTER_TEMPLETE_SELECTION_HELP indeed, plus it should be TEMPLATE - just shows that copy/paste is bad
fixing comment and misspelt macro
Sorry it took so long. I can fix the issues myself and upload a new patch for review, if you don't have time. Let me know - thanks! http://codereview.appspot.com/7855/diff/220/21 File include/gtest/internal/gtest-internal.h (right): http://codereview.appspot.com/7855/diff/220/21#newcode153 Line 153: #ifdef GTEST_COMPILER_NEEDS_POINTER_TEMPLATE_SELECTION_HELP This macro name is long and unclear. What does it mean by "pointer template selection"? What kind of help is needed? Since the compiler bug here is to require a copy constructor for ellipsis arguments, how about GTEST_ELLIPSIS_NEEDS_COPY_ ? Note the trailing _, which indicates that it's an internal macro and shouldn't be used by the user. http://codereview.appspot.com/7855/diff/220/21#newcode200 Line 200: #ifdef GTEST_COMPILER_NEEDS_POINTER_TEMPLATE_SELECTION_HELP I'm thinking maybe we can remove the #else branch below and remove this #ifdef as well. http://codereview.appspot.com/7855/diff/220/22 File include/gtest/internal/gtest-port.h (right): http://codereview.appspot.com/7855/diff/220/22#newcode70 Line 70: // GTEST_OS_CYGWIN - defined if compiled on Cygwin. "iff" means "if and only if". "A if B" means that B implies A, but "not B" doesn't necessarily imply "not A". "A iff B" is stronger. It means B implies A, and "bot B" implies "not A". Here we want "iff" as "not compiled on Cygwin" implies that GTEST_OS_CYGWIN is not defined. Can you change it back? http://codereview.appspot.com/7855/diff/220/22#newcode560 Line 560: #define GTEST_COMPILER_NEEDS_POINTER_TEMPLATE_SELECTION_HELP 1 It seems that this macro is used to control two different things: 1. whether ellipsis arguments require copy constructors, and 2. whether is_pointer is needed to help the compiler select the right overload. We should use two different macros for them. How about GTEST_ELLIPSIS_NEEDS_COPY_ and GTEST_NEEDS_IS_POINTER_ ? Actually, the latter macro may not be necessary after all. Can we just define is_pointer unconditionally? http://codereview.appspot.com/7855/diff/220/23 File src/gtest-filepath.cc (right): http://codereview.appspot.com/7855/diff/220/23#newcode54 Line 54: #define GTEST_PATH_MAX_ _MAX_PATH It's better to define this in gtest-port.h. http://codereview.appspot.com/7855/diff/220/23#newcode55 Line 55: #define getcwd _getcwd I've done this before, but think it was a hack. This has far reaching effects and can cause subtle problems. http://codereview.appspot.com/7855/diff/220/24 File src/gtest.cc (right): http://codereview.appspot.com/7855/diff/220/24#newcode108 Line 108: #if defined(GTEST_OS_ZOS) This shouldn't be separated from the cascading #elif defined(GTEST_OS_XXXX) branches starting at line 46. Move this to join that group, perhpas at line 68.
On 2008年11月06日 19:47:39, Zhanyong wrote: > I can fix the issues myself and upload a new patch for review, if you don't have > time. Let me know - thanks! > that would be awesome, thanks http://codereview.appspot.com/7855/diff/220/21 File include/gtest/internal/gtest-internal.h (right): http://codereview.appspot.com/7855/diff/220/21#newcode153 Line 153: #ifdef GTEST_COMPILER_NEEDS_POINTER_TEMPLATE_SELECTION_HELP On 2008年11月06日 19:47:39, Zhanyong wrote: > This macro name is long and unclear. What does it mean by "pointer template > selection"? What kind of help is needed? > > Since the compiler bug here is to require a copy constructor for ellipsis > arguments, how about > > GTEST_ELLIPSIS_NEEDS_COPY_ > > ? > > Note the trailing _, which indicates that it's an internal macro and shouldn't > be used by the user. fine by me http://codereview.appspot.com/7855/diff/220/21#newcode200 Line 200: #ifdef GTEST_COMPILER_NEEDS_POINTER_TEMPLATE_SELECTION_HELP On 2008年11月06日 19:47:39, Zhanyong wrote: > I'm thinking maybe we can remove the #else branch below and remove this #ifdef > as well. less branches is always good http://codereview.appspot.com/7855/diff/220/22 File include/gtest/internal/gtest-port.h (right): http://codereview.appspot.com/7855/diff/220/22#newcode70 Line 70: // GTEST_OS_CYGWIN - defined if compiled on Cygwin. On 2008年11月06日 19:47:39, Zhanyong wrote: > "iff" means "if and only if". > > "A if B" means that B implies A, but "not B" doesn't necessarily imply "not A". > > "A iff B" is stronger. It means B implies A, and "bot B" implies "not A". > > Here we want "iff" as "not compiled on Cygwin" implies that GTEST_OS_CYGWIN is > not defined. Can you change it back? never heard of that before - guess i should have googled beforehand. we simply call this an equivalence in german. http://codereview.appspot.com/7855/diff/220/22#newcode560 Line 560: #define GTEST_COMPILER_NEEDS_POINTER_TEMPLATE_SELECTION_HELP 1 On 2008年11月06日 19:47:39, Zhanyong wrote: > It seems that this macro is used to control two different things: > > 1. whether ellipsis arguments require copy constructors, and > 2. whether is_pointer is needed to help the compiler select the right overload. > > We should use two different macros for them. How about > > GTEST_ELLIPSIS_NEEDS_COPY_ > > and > > GTEST_NEEDS_IS_POINTER_ > > ? > > Actually, the latter macro may not be necessary after all. Can we just define > is_pointer unconditionally? macro names are fine. if gcc and msvc have no problem with is_pointer being defined, i see no problem either. http://codereview.appspot.com/7855/diff/220/23 File src/gtest-filepath.cc (right): http://codereview.appspot.com/7855/diff/220/23#newcode54 Line 54: #define GTEST_PATH_MAX_ _MAX_PATH On 2008年11月06日 19:47:39, Zhanyong wrote: > It's better to define this in gtest-port.h. agreed http://codereview.appspot.com/7855/diff/220/23#newcode55 Line 55: #define getcwd _getcwd On 2008年11月06日 19:47:39, Zhanyong wrote: > I've done this before, but think it was a hack. This has far reaching effects > and can cause subtle problems. well, i've taken it from the mailing list as you might have noticed. it sure is better than simply defining PATH_MAX if it isn't. http://codereview.appspot.com/7855/diff/220/24 File src/gtest.cc (right): http://codereview.appspot.com/7855/diff/220/24#newcode108 Line 108: #if defined(GTEST_OS_ZOS) On 2008年11月06日 19:47:39, Zhanyong wrote: > This shouldn't be separated from the cascading #elif defined(GTEST_OS_XXXX) > branches starting at line 46. Move this to join that group, perhpas at line 68. well, it now relies on the #else branch. we need the stuff in #else and strings.h.
Patch as proposed by Zhanyong via mailinglist
Patch as proposed by Zhanyong via mailinglist (fixed line endings)
http://codereview.appspot.com/7855/diff/237/417 File include/gtest/internal/gtest-port.h (right): http://codereview.appspot.com/7855/diff/237/417#newcode75 Line 75: // GTEST_OS_SOLARIS - defined iff compiled on Sun Solaris. Sorry I didn't notice this early - this list should be sorted alphabetically. Can you move this line before SYMBIAN? http://codereview.appspot.com/7855/diff/237/417#newcode570 Line 570: #endif Sorry I missed this - can you add a comment " // defined(__SYMBIAN32__) || defined(__IBMCPP__)"? Note that there should be exactly 2 spaces between "#endif" and "//". http://codereview.appspot.com/7855/diff/237/418 File src/gtest-filepath.cc (right): http://codereview.appspot.com/7855/diff/237/418#newcode54 Line 54: #define GTEST_PATH_MAX_ _MAX_PATH FYI: I tried to move the definition of GTEST_PATH_MAX_ to gtest-port.h, but found that it involves too much disruption (we need to move a bunch of headers that GTEST_PATH_MAX_ depends on there). Therefore I decided to keep it here. http://codereview.appspot.com/7855/diff/237/418#newcode55 Line 55: #elif defined(PATH_MAX) > > I was talking about "#define getcwd _getcwd". > > i actually think it's ok to do that, as we simply state 'on windows > use the microsoft specific implementation of that function'. i guess > one could additionally wrap it into a '#ifndef getcwd'. i do respect > you disliking this kind of pre-processor stuff though. The problem of "#define getcwd _getcwd" is that it has far-reaching effect: if you have a local variable, member function, or class/function in a namespace, and its name happens to be getcwd, it will be renamed silently to _getcwd. Worse, whether you get this renaming depends on whether you #include this header file before or after your code. This can be very frustrating. "#ifndef getcwd" won't work as getcwd is not a macro. Even if it is, this trick is evil as it leads to different definitions of the macro depending on the order in which you #include headers.
Rainer, Could you also add yourself to the CONTRIBUTORS file? Thanks!
Fixes as suggested by Zhanyong
Thanks, Rainer! This looks good. I'll work on checking it in soon. Cheers!
Thanks everybody for reviewing and providing valuable feedback! On 2008年11月10日 16:09:37, Zhanyong wrote: > Thanks, Rainer! This looks good. I'll work on checking it in soon. Cheers!