Moving the command line option variables to public

This patch makes the internal Google Test variables corresponding to the command line options available from the test source. It helps to changes the behavior of Google Test from the source instead of using the command line options or environment variables.

[Alexander, 2009-01-22 19:47:51]

I've moved the declarations of the Google Test command line option variables
from testing::internal to the public area making it visible from any test case
source and main(). Prior to this patch it was impossible to change option
directly, only using the command line switches or environment variables.

[Zhanyong Wan, 2009-01-22 22:54:44]

http://codereview.appspot.com/12483/diff/1/2
File include/gtest/gtest.h (right):

http://codereview.appspot.com/12483/diff/1/2#newcode96
Line 96: // Declares the flags.
Add comment before each flag to explain what it does.

http://codereview.appspot.com/12483/diff/1/2#newcode96
Line 96: // Declares the flags.
In gtest_unittest.cc, add a dedicated test right after "#include
<gtest/gtest.h>" to make sure that each of the following flags can be accessed
in code once <gtest/gtest.h> has been #included.

http://codereview.appspot.com/12483/diff/1/2#newcode102
Line 102: GTEST_DECLARE_bool_(death_test_use_fork);
This flag is temporary and may be removed later.  Therefore we shouldn't expose
it here.

http://codereview.appspot.com/12483/diff/1/2#newcode109
Line 109: GTEST_DECLARE_int32_(stack_trace_depth);
The above two flags were already declared below.

[Alexander, 2009-01-27 20:45:54]

Please review my patch with required changes.

[Zhanyong Wan, 2009-01-28 23:53:40]

Some nits.  Thanks!

http://codereview.appspot.com/12483/diff/9/202
File include/gtest/gtest.h (right):

http://codereview.appspot.com/12483/diff/9/202#newcode108
Line 108: // This flag enables using colors in terminal output.
List what the valid values are?

http://codereview.appspot.com/12483/diff/9/202#newcode111
Line 111: // This flag sets up the filter to select by name using regular
expression
regular expression => a glob pattern

http://codereview.appspot.com/12483/diff/9/202#newcode112
Line 112: // the tests to run. If the filter is not given all tests are
executes.
executes => executed

http://codereview.appspot.com/12483/diff/9/202#newcode116
Line 116: // are actually run if the flag is provided
Append a period.

http://codereview.appspot.com/12483/diff/9/202#newcode127
Line 127: // This flag sets how many times tests run is repeated. The default
value
"tests run is" => "the tests are"

http://codereview.appspot.com/12483/diff/9/202#newcode128
Line 128: // is 1. If the value is -1 tests are repeating forever.
tests => the tests

http://codereview.appspot.com/12483/diff/9/202#newcode132
Line 132: const int kMaxStackTraceDepth = 100;
This is now out-of-place.  Can you move it after the last flag declaration?

http://codereview.appspot.com/12483/diff/9/202#newcode140
Line 140: GTEST_DECLARE_bool_(show_internal_stack_frames);
The other flag declarations are sorted by their names.  Therefore move this
before stack_trace_depth?

http://codereview.appspot.com/12483/diff/9/203
File src/gtest-internal-inl.h (right):

http://codereview.appspot.com/12483/diff/9/203#newcode66
Line 66: GTEST_DECLARE_bool_(death_test_use_fork);
Why's this necessary?

http://codereview.appspot.com/12483/diff/9/201
File test/gtest_unittest.cc (right):

http://codereview.appspot.com/12483/diff/9/201#newcode37
Line 37: // This line tests that we can define tests in an unnamed namespace.
This is a distraction from the real property we are testing here.  I'd remove
the comment and the unnamed namespace.

http://codereview.appspot.com/12483/diff/9/201#newcode40
Line 40: // Tests CommandLineOptions.
This makes me think CommandLineOptions is a class or function.  I'd just remove
the comment - the next one is enough.

http://codereview.appspot.com/12483/diff/9/201#newcode42
Line 42: // Verifies that the command line option variables are accessed
option => flag

(for consistency)

are => can be

http://codereview.appspot.com/12483/diff/9/201#newcode43
Line 43: // in code once <gtest/gtest.h> has been #included.
Add a warning:

// Do not move it after other #includes.

http://codereview.appspot.com/12483/diff/9/201#newcode44
Line 44: TEST(CommandLineOptionsTest, Visibility) {
Options => Flags

Visibility => CanBeAccessedInCodeOnceGTestHIsIncluded

http://codereview.appspot.com/12483/diff/9/201#newcode45
Line 45: bool also_run_disabled_tests =
testing::GTEST_FLAG(also_run_disabled_tests);
This will cause some compilers to warn about unused variables.  Try to actually
use the value.

http://codereview.appspot.com/12483/diff/9/201#newcode45
Line 45: bool also_run_disabled_tests =
testing::GTEST_FLAG(also_run_disabled_tests);
The variable name is redundant and a distraction.  I prefer to just read the
flag values without assigning them to variables.  Perhaps

bool dummy = testing::GTEST_FLAG(...) && ... && testing::GTEST_FLAG(...);
EXPECT_TRUE(dummy || !dummy);

http://codereview.appspot.com/12483/diff/9/201#newcode48
Line 48: std::string color = testing::GTEST_FLAG(color);
This is a bit tricky: gtest needs to work where std::string is not available
(hence we defined our own testing::internal::String class).  Perhaps just

if (testing::GTEST_FLAG(color) != "unknown") {
  ...
}

The same for other std::string uses.

http://codereview.appspot.com/12483/diff/9/201#newcode56
Line 56: int stack_trace_depth = testing::GTEST_FLAG(stack_trace_depth);
Move this before show_internal_stack_frames to keep the list sorted.

[Alexander, 2009-01-30 00:59:54]

Please review my changes related to the review results.

http://codereview.appspot.com/12483/diff/9/203
File src/gtest-internal-inl.h (right):

http://codereview.appspot.com/12483/diff/9/203#newcode66
Line 66: GTEST_DECLARE_bool_(death_test_use_fork);
On 2009/01/28 23:53:40, Zhanyong Wan wrote:
> Why's this necessary?

Due to without this declaration the "death_test_use_fork" variable is undefined.

[Zhanyong Wan, 2009-01-30 01:34:30]

http://codereview.appspot.com/12483/diff/602/405
File src/gtest-internal-inl.h (right):

http://codereview.appspot.com/12483/diff/602/405#newcode63
Line 63: // We don't want the users to modify these flags in the code, but want
"these flags" => "this flag"

http://codereview.appspot.com/12483/diff/602/405#newcode64
Line 64: // Google Test's own unit tests to be able to access them.  Therefore
we
them => it

http://codereview.appspot.com/12483/diff/602/405#newcode65
Line 65: // declare them here as opposed to in gtest.h.
them => it

http://codereview.appspot.com/12483/diff/602/403
File test/gtest_unittest.cc (right):

http://codereview.appspot.com/12483/diff/602/403#newcode67
Line 67: EXPECT_TRUE(dummy_int || !dummy_int);
What I meant is to write:

bool dummy = testing::GTEST_FLAG(also_run_disabled_tests)
  || testing::GTEST_FLAG(break_on_failure)
  ...
  || testing::GTEST_FLAG(repeat) > 0
  ...
  || testing::GTEST_FLAG(filter) != "unknown"
  ...;
EXPECT_TRUE(dummy || !dummy);

Only two statements are needed.  This way it's very easy for a reader to verify
that each flag has been exercised.

[Alexander, 2009-01-30 23:26:42]

Please review my patch with new corrections.