Static analysis results of Catch-1

[martinmoene]

Perhaps the analyses contain something of interest.

martinmoene/CppCheck.txt.

Contains

• CppCheck
• PVS-Studio
• Resharper-C++
• VC-CppCoreCheck-released.txt
• VC-CppCoreCheck-experimental.txt

Please let me know if you like a different format.

[horenmar]

Passing notes from looking through this:

CppCheck and Resharper seem to run into trouble with how Catch-1 is structured. Hopefully they would do better with Catch 2, where implementations are split out into .cpp files, but going through them like this is kinda painful.

Resharper loves to say "use this c++11 feature in your C++98 codebase". Ironically, when Catch is properly configured to support C++11, some of these (overrides, noexcepts) are already used.

Actionable finds:

CppCheck

..\..\..\..\..\..\Local\Project\_GitHub\Catch\single_include\catch.hpp|500|copyCtorAndEqOperator : style : The class 'NotImplementedException' has 'copy constructor' but lack of 'operator='.|
..\..\..\..\..\..\Local\Project\_GitHub\Catch\single_include\catch.hpp|2528|copyCtorAndEqOperator : style : The class 'CompositeGenerator' has 'copy constructor' but lack of 'operator='.|
..\..\..\..\..\..\Local\Project\_GitHub\Catch\single_include\catch.hpp|2770|copyCtorAndEqOperator : style : The class 'Approx' has 'copy constructor' but lack of 'operator='.|

Deleted the constructors for Approx and NotImplementedException. Neither of those would cause a problem, because the default generated operator= would do the right thing, but there is no reason to keep them explicitly. CompositeGenerator has move semantics in copy constructor going on and no operator=, which is weird, but generators are a very vestigial part of Catch 1 (never documented or officially supported) and their current implementation is gone in Catch 2, so I am not touching that.

I didn't go through all the "never used" warnings, because I know that some of them are used, so it is likely more false positives.

PVS Studio

V817 It is more efficient to seek '.' character rather than a string. SelfTest catch_session.hpp 98

No reason not to fix this. The others are things that we might want to suppress in the future, but basically false positives. (The throw warning is caused by indirection that it doesn't quite see through, the pointer cast is necessary evil for REQUIRE( ptr == NULL ) to work).

Resharper-C++

Goes completely crazy in face of obj-c. Used it to get rid of superfluous inlines in Catch 1 (did so in Catch 2 some time ago) and otherwise skipped it, as already mentioned it loves to say "use auto in C++98 codebase", so there is too much reported.

Core Guidelines

Skipped for now.

---

All in all, relatively useful but would probably work better against Catch 2, so some of the tools wouldn't be horribly confused by Catch being implemented in headers only, and so all the "use override, auto, constexpr" would be actionable (and also wouldn't be used in places where they already are used hidden in macro).

Fixed couple problems, going to fix couple more*

• Catch is currently Valgrind and sanitizer clean, so the uninitialized variable warnings are not real bugs, but I would prefer them not to be there. However I am going to need some time to decide how to best tackle them.

[martinmoene]

@horenmar Would you be interested in reports on Catch 2, stripped from the 'less useful' warnings?

[martinmoene]

@horenmar Likely of interest are 238 of 239 ReSharper C++ mentions of non-inline function definition in a header file. Not expecting to find more than 1 main() in the collection of translation units that make up a test executable ;)

[60/61 in Catch 2]

Edit: Apparently this isn't a real problem, otherwise it would likely already have led to violation of the ODR. Perhaps it's ok because of the way Catch is structured, separating Catch implementation and Catch interface.

[martinmoene]

@horenmar Mismatched class tag in catch.hpp, 6 times.

s/class/struct/ in

class Evaluator{};

template<typename T1, typename T2>
struct Evaluator<T1, T2, IsEqualTo> 

[horenmar]

@martinmoene If the warnings were pared down a bit, it would be much nicer to work with (and also have better chance to be fully addressed).

And yeah, due to how Catch 1 is written, there are non-inline function bodies in headers all over, and this won't change until Catch 2, where we are separating non-template definitions into .cpp files (that are then also stitched into the single-header version, so running the analysis over that would still find them in error).

[martinmoene]

@horenmar I'll rerun (some of) the analyses, reduce their output and republish them, hopefully tonight.

[martinmoene]

@horenmar I've updated the results for Resharper-C++ (now 108 lines).

[martinmoene]

Catch is currently Valgrind and sanitizer clean, so the uninitialized variable warnings are not real bugs, but I would prefer them not to be there. However I am going to need some time to decide how to best tackle them.

Do you run these locally, or via Travis (I couldn't spot them)?

[horenmar]

Locally only. If you feel like making change to Travis to also run SelfTest under Valgrind/Sanitizers, it would be most welcome.

[martinmoene]

If only I'd have experience with it...