This warning is generated when a type value that is not bool is assigned or coerced into type bool. Typically, this message is caused by assigning int variables to bool variables where the int variable contains only values true and false, and could be re declared as type bool. Ii is generally recommended to add "(0 != X)" to the expression, which gives the expression the type bool.
For more info see: http://msdn.microsoft.com/en-us/library/b6801kcy.aspx
Generally looks good, though I'm not such a fan of the yoda conditionals. Also, the extra parenthesis on many lines makes it harder to read imo.
Agree on the yoda conditionals. We don't use this anywhere, so let's not start introducing them.
Indeed -- please make the code look like other bitcoin code. We never use "(0 == ...)".
Personally I consider not using yoda conditionals in C and C++ poor coding style. If you work for heavy C++ coders like Hughes or IBM you would be frowned upon for that. But if you insist, have it your way...
The even-more-universal rule applies: Your code should look like the code around it. This applies to any codebase. In this case, your code should look like bitcoin code, not IBM or Hughes code. :)
@jgarzik If code does not follow good practices and coding standards it should be improved. Should I mention not being able to build release variant because of assert()? Lets face it, BC could benefit from some code improvements.
It is kind of strange to hear about shortage of developers supporting core dev for btc on each and every conference and being snubbed by the team when this help is offered on the grounds that codding "good practices" go against personal preferences.
Coding style preferences, even Hughes or IBM's, are not "good practices".
Nobody will claim that the source code is perfect. There are many improvements possible, and all help is welcome (though the bottleneck is testing and reviewing).
Still, please respect that each code base has its own style, and that that is really just a convention.
WRT Yoda conditions specifically: there once was a good reason for them (avoiding accidental assignment when you wanted a test), but every compiler nowadays will warn you for that anyway. To anyone familiar with this code now, it just looks confusing. BTW: I don't think I ever saw a Yoda condition in the Google code base.
Thanks for changing it :)
Does this need to be 5 commits or can they be merged into one?
493 | @@ -494,7 +494,7 @@ bool RenameOver(boost::filesystem::path src, boost::filesystem::path dest) 494 | { 495 | #ifdef WIN32 496 | return MoveFileExA(src.string().c_str(), dest.string().c_str(), 497 | - MOVEFILE_REPLACE_EXISTING); 498 | + MOVEFILE_REPLACE_EXISTING) != 0;
extraneous space
And rightfully so; the indentation was wrong...
Now that everyone seams to be happy. Squashed.
I've worked on various C and C++ projects for companies and never saw 'yoda conditionals' in serious use. I have seen my share of other (sometimes crazy) "let's use this coding style to avoid bugs" initiatives though, so I've seen where you're coming from. However there is no evidence that religiously insisting on a certain coding style actually avoids bugs. Usually these were projects riddled with bugs because the developers at the organization spent all their time writing reports and checklists instead of paying attention to the code.
Anyhow -- thanks for showing some flexibility and changing this. One final nit: the commit message, "Squashed all into one" is funny but says little :)
Untested ACK
Why is it failing now, there are no changes been made? Is there a way to re run the check other than to make new commit?
ut ACK
Oh, right; change the commit message to something meaningful, please.
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4825_98fbd76868f8610a805da2566235e5b643f15434/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.