Suppress Clang-Tidy warnings

[hanspacket]

When we run clang-tidy on our source code, it finds hundreds of issues related to usages of gtest. We currently have to disable a few clang-tidy checks because of this. In the longer run, we want to enable these checks again. I believe the reported issues are false positives, so it would be beneficial for us if gtest would suppress these warnings.

I found out (among others in https://github.com/google/googletest/issues/853) that gtest already contains 'NOLINT' comments to suppress clang-tidy warnings. I would like to point out a few additional places to add them (based on the code in version 1.8.1):

• clang-tidy warning 'cert-err58-cpp'
  • gtest_internal.h
    • affects usages of TEST and TEST_F
    • add nolint comment to the assignment on line 1319
  • gtest-param-test.h
    • affects usages of TEST_P and INSTANTIATE_TEST_CASE_P
    • add nolint comment to the assignment on lines 1396/1397
    • add nolint comment to the assignment on line 1421
• clang-tidy warning 'cppcoreguidelines-avoid-goto'
  • gtest-internal.h
    • affects usages of EXPECT_NO_THROW and similar
    • add nolint comment to lines 1231, 1237, 1250, 1268, 1294
  • gtest-death-test-internal.h
    • affects usages of EXPECT_DEATH and similar
    • add nolint comment to lines 196, 204

If I can help out in any other way, let me know.

[Zitrax]

Two more I hit:

• cppcoreguidelines-owning-memory
  • gtest.h - (initializing non-owner argument of type 'testing::internal::TestFactoryBase *' with a newly created 'gsl::owner<>' )
• cppcoreguidelines-special-member-functions
  • gtest-param-test.h - (class 'SleepTest_Sleep_Test' defines a copy constructor and a copy assignment operator but does not define a destructor, a move constructor or a move assignment operator)

[andrew-wils]

One more:

• cppcoreguidelines-avoid-non-const-global-variables
  • gtest.h: - Variable 'test_info_' provides global access to a non-const object; consider making the pointed-to data 'const'

[Trass3r]

* clang-tidy warning 'cert-err58-cpp'

Would noexcept help with that?

[jaques-sam]

@hanspacket

If I can help out in any other way, let me know.

You already know where to place the NOLINTs, just go ahead! This has been open for too long 😉

[TheStormN]

This is again about 'test_info_' but with different(more recent) error: Clang-Tidy: Initialization of 'test_info_' with static storage duration may throw an exception that cannot be caught.

Located in gtest-internal.h. I would've submit a PR, but no matter where I put the NOLINT I cannot suppress the warning. It's a complicated macro stuff, so if someone have a hint what exactly should be modified, that would be great.

[harrism]

Hi @derekmauro , I have a change that fixes and/or NOLINTS several of these clang-tidy warnings/errors. I would like to contribute this as a (partial?) fix for this issue, but I wanted to check with the maintainers before I submit a PR. Is this something you would be supportive of? We're adding clang-tidy checking in our CI in RAPIDS at NVIDIA and we need to suppress the many warnings/errors we get in our test code due to gtest. It's not possible to suppress them in our code because of the use of preprocessor macros.

Here is my branch: https://github.com/harrism/googletest/tree/fix-clang-tidy-nolint

[OleksandrKvl]

Hi, I tried to fix all the warnings, check out #3676. Try it and let me know if something is still not fixed.

[aminya]

I switched to catch2 because of this issue. It does not have any warnings internally.

[avdg81]

Hi everyone, I am facing the exact same issue as described here. Since the problem report was opened back in September 2019, I'm curious about its current status. Can we expect that a fix will be made available any time soon (some have already suggested solutions for it)? Or is there something that I may not be aware of which is blocking its resolution? Any feedback is greatly appreciated. Thank you.

[harrism]

Can we expect that a fix will be made available any time soon

Doubtful: https://github.com/google/googletest/pull/3676#issuecomment-1030228060

[martin00001313]

Seems this will not be fixed/updated by clang for a while as it is considered as tool specific error.