Suppress Clang-Tidy warnings
Opened this issue · 11 comments
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 #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
- gtest_internal.h
- 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
- gtest-internal.h
If I can help out in any other way, let me know.
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)
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'
* clang-tidy warning 'cert-err58-cpp'
Would noexcept help with that?
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 😉
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.
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
Hi, I tried to fix all the warnings, check out #3676. Try it and let me know if something is still not fixed.
I switched to catch2 because of this issue. It does not have any warnings internally.
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.
Can we expect that a fix will be made available any time soon
Doubtful: #3676 (comment)
Seems this will not be fixed/updated by clang for a while as it is considered as tool specific error.