`TYPED_TEST_SUITE_P` and `-Wexit-time-destructors`
- 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
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