How to properly rewrite ASSERT code to pass /analyze in msvc?

Question

Visual Studio added code analysis (/analyze) for C/C++ in order to help identify bad code. This is quite a nice feature but when you deal with and old project you may be overwhelmed by the number of warnings.

Most of the problems are generating because the old code is doing some ASSERT at the beginning of the method or function.

I think this is the ASSERT definition used in the code (from afx.h)

#define ASSERT(f)          DEBUG_ONLY((void) ((f) || !::AfxAssertFailedLine(THIS_FILE, __LINE__) || (AfxDebugBreak(), 0)))

Example code:

ASSERT(pBytes != NULL);
*pBytes = 0; // <- warning C6011: Dereferencing NULL pointer 'pBytes'

I'm looking for an easy, clean and safe solution to solve these warnings that does not imply disabling these warnings. Did I mention that there are lots of occurrences in current codebase?

Answer 1

/analyze is not guaranteed to yield relevant and correct warnings. It can and will miss a lot of issues, and it also gives a number of false positives (things it identifies as warnings, but which are perfectly safe and will never actually occur)

It is unrealistic to expect to have zero warnings with /analyze.

It has pointed out a situation where you dereference a pointer which it can not verify is always valid. As far as PREfast can tell, there's no guarantee that it will never be NULL.

But that doesn't mean it can be NULL. Just that the analysis required to prove that it's safe is too complex for PREfast.

You may be able to use the Microsoft-specific extension __assume to tell the compiler that it shouldn't produce this warning, but a better solution is to leave the warning. Every time you compile with /analyze (which need not be every time you compile), you should verify that the warnings it does come up with are still false positives.

If you use your asserts correctly (to catch logic error during programming, guarding against situations that cannot happen, then I see no problem with your code, or with leaving the warning. Adding code to handle a problem that can never occur is just pointless. You're adding more code and more complexity for no reason (if it can never occur, then you have no way of recovering from it, because you have absolutely no clue what state the program will be in. All you know is that it has entered a code path you thought impossible.

However, if you use your assert as actual error handling (the value can be NULL in exceptional cases, you just expect that it won't happen), then it is a defect in your code. Then proper error handling (exceptions, typically) is needed.

Never ever use asserts for problems that are possible. Use them to verify that the impossible isn't happening. And when /analyze gives you warnings, look at them. If it is a false positive, ignore it (don't suppress it, because while it's a false positive today, the code you check in tomorrow may turn it into a real issue).