Clang cleanup #2123

Closed
wants to merge 2 commits into
from

6 participants

@apoelstra

Yesterday LLVM 3.2 was released, as well as a corresponding version of the clang compiler and static analyzer.

I ran the static analyzer over the bitcoin code and picked up a couple of assignments to variables that are never read. These two patches remove those assigments.

@BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/75fafc30d24648588801baa50242c6d05cd5d75b for binaries and test log.

@Diapolo Diapolo commented on an outdated diff Dec 22, 2012
src/serialize.h
@@ -1045,10 +1045,7 @@ class CDataStream
if (nReadPosNext >= vch.size())
{
if (nReadPosNext > vch.size())
- {
- setstate(std::ios::failbit, "CDataStream::ignore() : end of data");
- nSize = vch.size() - nReadPos;
- }
+ setstate(std::ios::failbit, "CDataStream::ignore() : end of data");
@Diapolo
Diapolo added a line comment Dec 22, 2012

This needs 4 spaced as indention :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@BitcoinPullTester

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/3d12bdaa7ce61bc6dbe395a7f083325f041b4a00 for binaries and test log.

@Diapolo

I'm interested in how that analyzer outputs found stuff, can you perhaps post that? As I'm on Windows it's not that easy to get such stuff working over here ;).

@apoelstra

On Linux, you can get the analyzer to run by adding

CXX=clang++ --analyze

right above the "LINK=$(CXX)" line in makefile.unix, then do a full build. It will output its warnings as though it were a compiler (though it won't actually compile anything). I'm not familiar with the Windows build process, sorry.

@laanwj
Bitcoin member

I've also seen these warnings by the analyzer, but I'm not so sure about removing the code. It's there to make maintenance easier (ie, will make it somewhat more robust when adding code by making sure hSOCKET will never have an invalid value, and nSize will always be up to date). Compilers are generally smart enough to remove dead code like this in the binary.

@sipa
Bitcoin member

agree with @laanwj

Being able to get useful information through a static analyzer is nice, though.

@Diapolo

It would be indeed better to keep these 2 and so I'm fine with just closing this.

@gavinandresen
Bitcoin member

Closing this, I like the belt-and-suspenders easier-to-maintain code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment