Fix Clang warnings

[dsacre]

This branch fixes a series of warnings that occur when building with Clang, with the -Wdeprecated and -Wmissing-variable-declarations flags enabled.

[gennadiycivil]

@dsacre Thank you for this contribution.

googletest can not suport all warning flags combinations for all compilers. Given the wide usage this is not practical nor needed.

[dsacre]

@gennadiycivil These changes are not to appease a single overzealous compiler, but address actual (albeit minor) code quality issues in Google Test. For example, ValueArray's use of an implicitly defined copy constructor has been deprecated by the language standard since C++11.

I'd also like to mention that due to Google Test's heavy use of preprocessor macros, some of these warnings "leak" into user code. In other words, there is no way to silence these warnings without disabling the corresponding compiler flags for the user's code base as well.

[gennadiycivil]

@dsacre I am re-opening this PR, could you please provide more information with real use cases / test cases to support "some of these warnings "leak" into user code. In other words, there is no way to silence these warnings without disabling the corresponding compiler flags for the user's code base as well.".
Lets see if we can figure it out

[dsacre]

@gennadiycivil I appreciate you taking another look at this.

What I mean by "leaking" is this: Normally, if you don't want to see warnings from a third-party library while compiling your own code (with warnings of your choice enabled), you have two options: Specify the library's header search path with -isystem, or surround the #include statement with a few #pragma directives:

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wmissing-variable-declarations"
#include "gtest/gtest.h"
#pragma clang diagnostic pop

However, in some cases (specifically the issue fixed by 1811dd2) neither option will work, because the offending code is inside a macro. By the time the compiler gets to it, the preprocessor will already have inserted it into my own code every time I use TYPED_TEST().

The other instances of -Wmissing-variable-declarations don't leak like that, but still look like oversights to me.

I'm a little on the fence about fd38c01. Clang basically warns about a rule-of-three violation, but apparently the only reason copy-assignment is forbidden were warnings from MSVC. That commit was nine years ago, so I wonder if it's even still relevant.

[hadrielk]

I haven't looked into what this ValueArray does, so I could easily be wrong, but couldn't this line just be:

ValueArray1(const ValueArray1&) = default;

because this explicit copy constructor sure looks like what = default; would generate from the compiler. In which case every other change in this commit could also be a single = default; line, and not have to explicitly copy each member of other into *this based on the particular ValueArray. No?

[pavelkryukov]

It is a C++11 feature but GoogleTest keeps compatibility with C++03.

[hadrielk]

At my company we also hit the issue being fixed in 1811dd2 all the time when using parameterized tests. We usually end up wrapping the entire test in an anonymous namespace to avoid it, but it would be nice not to have to (remember to) do that.

[gennadiycivil]

@dsacre @hadrielk @pavelkryukov Apologies for the wait. Could you please resolve conflicts.

[dsacre]

@gennadiycivil I've rebased the branch onto master, and also amended one of the commits to add another missing declaration for a flag that was recently introduced.

[gennadiycivil]

@dsacre Thank you for the changes. Please provide test run output "before" and "after" that clearly illustrates the changes and their results
Thanks again

[dsacre]

@gennadiycivil This is the output I get when compiling Google Test itself (the flags -Wmissing-variable-declarations and -Wdeprecated, among others, are inherited via CMake):

googletest/src/gtest.cc:243:1: warning: no previous extern declaration for non-static variable 'FLAGS_gtest_install_failure_signal_handler' [-Wmissing-variable-declarations]
GTEST_DEFINE_bool_(
^
googletest/include/gtest/internal/gtest-port.h:2670:21: note: expanded from macro 'GTEST_DEFINE_bool_'
    GTEST_API_ bool GTEST_FLAG(name) = (default_val)
                    ^
googletest/include/gtest/internal/gtest-port.h:2651:27: note: expanded from macro 'GTEST_FLAG'
# define GTEST_FLAG(name) FLAGS_gtest_##name
                          ^
<scratch space>:275:1: note: expanded from here
FLAGS_gtest_install_failure_signal_handler
^
googletest/src/gtest.cc:327:1: warning: no previous extern declaration for non-static variable 'FLAGS_gtest_flagfile' [-Wmissing-variable-declarations]
GTEST_DEFINE_string_(
^
googletest/include/gtest/internal/gtest-port.h:2674:30: note: expanded from macro 'GTEST_DEFINE_string_'
    GTEST_API_ ::std::string GTEST_FLAG(name) = (default_val)
                             ^
googletest/include/gtest/internal/gtest-port.h:2651:27: note: expanded from macro 'GTEST_FLAG'
# define GTEST_FLAG(name) FLAGS_gtest_##name
                          ^
<scratch space>:287:1: note: expanded from here
FLAGS_gtest_flagfile
^
googletest/src/gtest.cc:415:28: warning: no previous extern declaration for non-static variable 'g_argvs' [-Wmissing-variable-declarations]
::std::vector<std::string> g_argvs;
                           ^
In file included from googletest/src/gtest.cc:33:
In file included from googletest/include/gtest/gtest.h:63:
In file included from googletest/include/gtest/gtest-param-test.h:190:
googletest/include/gtest/internal/gtest-param-util-generated.h:103:8: warning: definition of implicit copy constructor for 'ValueArray2<bool, bool>' is deprecated because it has a user-declared
      copy assignment operator [-Wdeprecated]
  void operator=(const ValueArray2& other);
       ^
googletest/include/gtest/gtest-param-test.h:349:10: note: in implicit copy constructor for 'testing::internal::ValueArray2<bool, bool>' first required here
  return internal::ValueArray2<T1, T2>(v1, v2);
         ^
googletest/include/gtest/gtest-param-test.h:1216:10: note: in instantiation of function template specialization 'testing::Values<bool, bool>' requested here
  return Values(false, true);
         ^

Additionally, when compiling the following example code:

#include "gtest/gtest.h"

template <typename T>
class FooTest : public ::testing::Test { /*...*/ };

using MyTypes = ::testing::Types<char, int>;
TYPED_TEST_CASE(FooTest, MyTypes);

TYPED_TEST(FooTest, HasPropertyA) { /*...*/ }

...this is what Clang says:

test.cpp:9:1: warning: no previous extern declaration for non-static variable 'gtest_FooTest_HasPropertyA_registered_' [-Wmissing-variable-declarations]
TYPED_TEST(FooTest, HasPropertyA) { /*...*/ }
^
googletest/include/gtest/gtest-typed-test.h:179:8: note: expanded from macro 'TYPED_TEST'
  bool gtest_##CaseName##_##TestName##_registered_ GTEST_ATTRIBUTE_UNUSED_ = \
       ^
<scratch space>:44:1: note: expanded from here
gtest_FooTest_HasPropertyA_registered_
^
In file included from test.cpp:1:
In file included from googletest/include/gtest/gtest.h:63:
In file included from googletest/include/gtest/gtest-param-test.h:190:
googletest/include/gtest/internal/gtest-param-util-generated.h:103:8: warning: definition of implicit copy constructor for 'ValueArray2<bool, bool>' is deprecated because it has a user-declared
      copy assignment operator [-Wdeprecated]
  void operator=(const ValueArray2& other);
       ^
googletest/include/gtest/gtest-param-test.h:349:10: note: in implicit copy constructor for 'testing::internal::ValueArray2<bool, bool>' first required here
  return internal::ValueArray2<T1, T2>(v1, v2);
         ^
googletest/include/gtest/gtest-param-test.h:1216:10: note: in instantiation of function template specialization 'testing::Values<bool, bool>' requested here
  return Values(false, true);
         ^

With the proposed changes, the compiler is silent in both cases.

[gennadiycivil]

@dsacre Could you please resolve conflicts

[dsacre]

@gennadiycivil done, rebased on current master.