Hi,
I've been working with Johan and Bryce on the clang and Coverity analysis. Bryce should be getting the coverity runs more automated soon, but we'll take a little before things get published. We'll need to ensure we are sensitive to security, disclosure, etc.
However I do have a few preliminary results. First goes to leveraging the clang results effectively. We'll need to pull together some wiki info on which clang issues to prioritize, and what approaches might be used for remediation of some of the reported problems. Initially we have one question on "Dead assignment" and the other dead store. The first case Johan and I looked at was one that did warrant skipping initialization... however clang reports 167 of those while Coverity lists only 8 issues that might overlap them. So this set of clang warnings appears to be a bit subjective on the clang dev's part, or perhaps just more advisory. So we should probably look at addressing those last.
In the Coverity results I spotted two different big-picture issues hiding in the details. One warrants just low-level code cleanup while the other should be looked over by as many people keeping 'architecture' in mind.
The simple issue is with the GTK-style casting macros such as SP_IS_USE(x) and SP_USE(x). They started out following the GTK team's approach to adding objects to C. A while back these were refactored to be C++ oriented so existing code would not have to be changed as we migrated to true classes. However, the usage is backwards (check-then-cast vs. cast-then-check) and conceptually there are some big mismatches that lead to most usages being vulnerable to null pointer dereferencing. Conceptually these are also backwards as they use run-time checks instead of compile-time checks.
From an implementation viewpoint, these macros have two major problems.
First is that they cast away any const attributes and thus make the code less safe. Secondly (and far more critical) they are using C-Style casts that also make them take any random block of data and treat it as a proper C++ object. One of my test cases was to feed the macros a simple array of 5 arbitrary bytes. I expected it to accept but result in NULL, however it instead gave me a seemingly valid SPUse pointer. Not good.
The good news is that in the vast majority of cases, no macros or casts are needed at all in C++ (for upcasting, etc.) and thus their use can just be removed. I'm working through an initial cleanup pass that should correct all but those where the improper loss of 'const' comes into play. This cleanup is good since these macros are responsible for 600-800 of the outstanding 1,135 Coverity defects.
The second issue is a higher-level problem of design and naming. There are a few methods who's names appear to be leading to buggy use. The main example is SPShape::getCurve(). The problem is that to convey proper meaning this probably should be called something more like SPShape::generateNewCurve(). Many developers were treating it similar to std::string::c_str() and accessing it to query attributes of the resulting curve without explicitly grabbing and then freeing the curve. That is, the name implied access to an existing object instead of creation and ownership transfer of a new object.
To fix this we might simply change the naming approach (e.g. avoid 'getFoo()' for anything that allocates) and the using code... or we could change getCurve() and friends to operate on an internal object that is persistent... or perhaps something else.