Addressing static analysis from coverity scan #511
Conversation
| * @param hint additional exception information | ||
| */ | ||
| template <class T> | ||
| void CheckForNullptr(const T *pointer, const std::string hint); |
There was a problem hiding this comment.
Since this is only checking for null then the pointer type doesn't matter and this can just as easily be void* instead of a template ight? The body of the function doesn't use any type information.
There was a problem hiding this comment.
this function is inlined...T can be anything. I don't want to introduce void* unless it's the only option.
| ::testing::InitGoogleTest(&argc, argv); | ||
| int result = RUN_ALL_TESTS(); | ||
| int result = -1; | ||
| try |
There was a problem hiding this comment.
Why swallow the exception here rather than propagate it?
There was a problem hiding this comment.
Coverity is complaining about RUN_ALL_TEST not being caught.
There was a problem hiding this comment.
That's okay. It's allowed to complain about some things. Test in particular we want to crash if an exception makes it that fer since that means we didn't catch it internally where we should have. I's rather keep the exception propagation here and teach coverity to ignore that specific issue
There was a problem hiding this comment.
Let's do the latter....I will change this back.
There was a problem hiding this comment.
It's part of getting used to both false alarms and acceptable errors with automated analysis tooling. 100% pass rate is never expected and in this case, even if we can't easily suppress it, I'd say it's an acceptable error.
There was a problem hiding this comment.
@chuckatkins so I just rerun the tests with intentional failures and the try-catch doesn't affect the end result. This is just catching an exception thrown by google-test itself, not the tests generated exceptions.
There was a problem hiding this comment.
Still though, we don't want to swallow exceptions, especially in test code. If an exception get's thrown by the test framework then the test should crash. Coverity flagging it as an issue is a generic "this could crash" type error, which in this case, is known and intended.
There was a problem hiding this comment.
That's exactly my point in the previous comment about intentional failures, tests exceptions don't get swallowed by this try-catch in main. They are always caught inside gtest, which is intended. You can try to reproduce on your end. This Coverity flag is almost half of the report. It's not about crashes, it's about catching the gtest library exception (not the tests generated ones)...a crash is a crash, regardless. I think it's worth removing it to focus on the important pieces.
There was a problem hiding this comment.
It's caught as a possible error by Coverity but that doesn't mean it needs to, or in this case, even should be fixed. If it is indeed the case that an exception being thrown internally by gtest propagates up to main then something has gone very wrong and the test should crash. It could also be a false alarm by Coverity. A global try-catch just to make the scan report clean hides the issue in the actual code, if there is one at all (which we don't know atm). The global try-catch in test code main's should be removed and instead addressed via a modeling file in the coverity project page.
There was a problem hiding this comment.
I think we are going in circles here. If you can't provide an example on where this fails and if it can't be fixed with commenting out 2 lines of code then I don't think it's worth spending more time on this.
No description provided.