google / google/googletest

`TYPED_TEST_SUITE_P` and `-Wexit-time-destructors`

Open
#4,803 0 comments 0 reactions 0 assignees View on GitHub
enhancement
Dominant language
C++
Stars
39.5k
Forks
10.9k
Avg merge
6d 13h
Merged PRs (30d)
1

Description

### Does the feature exist in the most recent commit?

No.

### Why do we need this feature?

I have been going about in Chromium, trying to make clang's `-Wexit-time-destructors` a default enabled warning, in order to reduce the risk with destruction time ordering issues. Upon further investigation, I noticed that some of the test that are coming back with warnings happen to trip the warning whenever they use `TYPED_TEST_SUITE_P`. This macro looks like this:

```cxx
// The variables defined in the type-parameterized test macros are
// static as typically these macros are used in a .h file that can be
// #included in multiple translation units linked together.
#define TYPED_TEST_SUITE_P(SuiteName) \
static ::testing::internal::TypedTestSuitePState \
GTEST_TYPED_TEST_SUITE_P_STATE_(SuiteName)
```

And `::testing::internal::TypedTestSuitePState` looks like this:

```cxx
// State of the definition of a type-parameterized test suite.
class GTEST_API_ TypedTestSuitePState {
public:
TypedTestSuitePState() : registered_(false) {}

// Adds the given test name to defined_test_names_ and return true
// if the test suite hasn't been registered; otherwise aborts the
// program.
bool AddTestName(const char* file, int line, const char* case_name,
const char* test_name) {
if (registered_) {
fprintf(stderr,
"%s Test %s must be defined before "
"REGISTER_TYPED_TEST_SUITE_P(%s, ...).\n",
FormatFileLocation(file, line).c_str(), test_name, case_name);
fflush(stderr);
posix::Abort();
}
registered_tests_.emplace(test_name, CodeLocation(file, line));
return true;
}

bool TestExists(const std::string& test_name) const {
return registered_tests_.count(test_name) > 0;
}

const CodeLocation& GetCodeLocation(const std::string& test_name) const {
RegisteredTestsMap::const_iterator it = registered_tests_.find(test_name);
GTEST_CHECK_(it != registered_tests_.end());
return it->second;
}

// Verifies that registered_tests match the test names in
// defined_test_names_; returns registered_tests if successful, or
// aborts the program otherwise.
const char* VerifyRegisteredTestNames(const char* test_suite_name,
const char* file, int line,
const char* registered_tests);

private:
typedef ::std::map> RegisteredTestsMap;

bool registered_;
RegisteredTestsMap registered_tests_;
};
```

A global static for this type requires non-trivial destructors, and although the use by gtest is safe here, that ends up interfering with enforcing this warning elsewhere.

### Describe the proposal.

There are two ways to make googletest not interfere with this warning:

a) We make these static objects use some sort of NoDestructor type, similar to chromium's `base::NoDestructor` or abseil's `NoDestructor`. Notice provides an attribute for this too, namely, `[[clang::no_destroy]]` (https://clang.llvm.org/docs/AttributeReference.html#no-destroy)

b) We add a macro with for clang, with a compile pragma, blessing in particular `TYPED_TEST_SUITE_P` and other similar macros.

I think b is more realistic, and I could even help implementing it, if that's something that would be acceptable.

### Is the feature specific to an operating system, compiler, or build system version?

It is specific to clang with `-Wexit-time-destructors`.

Contributor guide

Open the contributing guide

Research direction

Start by locating the TYPED_TEST_SUITE_P macro and TypedTestSuitePState definition shown in the issue, then reproduce the clang -Wexit-time-destructors warning with the relevant typed-test macros. Compare the proposed no-destroy and clang pragma approaches, and confirm that the chosen change removes the warning without breaking typed-parameterized tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers, testing
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.