search

accre / lstore

LStore - A fault-tolerant, performant distributed data storage framework.
http://www.lstore.org
Apache License 2.0
4 stars 5 forks source link

Clean compiler errors #61

Closed PerilousApricot closed 8 years ago

PerilousApricot commented 8 years ago

We need to do what we can to drop these compiler errors down to a minimum/a zero:

http://jenkins.accre.vanderbilt.edu/job/LStore-Master/lastSuccessfulBuild/warnings23Result/

I believe @tacketar said he'd pick it up

tacketar commented 8 years ago

And these are not actually errors. The first one I glanced at will require superflous code. Just because we don't check the return code directly doesn't mean another side effect isn't used to more directly probe the result.

PerilousApricot commented 8 years ago

Maybe we're looking at different things. There's a ton of errors that aren't just -Wsign-compare or -Wunused-result

tacketar commented 8 years ago

It's been a while since I looked at the clang output but I do recognize several of the non-trivial ones as false positives in the static analyzer. I remember several of them were due to split brain assumptions on loops with the only way of making it go away was to add extra logic to the loop who's sole purpose was to trap an impossible scenario. I'll take another look at it in any case.

PerilousApricot commented 8 years ago

This is actually from GCC5.3 (though addling a clang version would be helpful since they're different implementations).

In either case, please look through and jump through whatever hoops it takes so we can enable -Werror.

You might think it's the dumbest thing ever, but from experience with much larger projects (in terms of LOC and developers) for CMS, having a test suite where you have to mentally ignore certain errors in the code because they were there for historical reasons made it so the whole test suite got discounted, and bugs slip through the cracks left and right. It's one of my bigger regrets, development-wise.

tacketar commented 8 years ago

I draw the line on adding superflous code in a tight loop just to make the compiler happy though. Although I'm perfectly happy adding a comment above it saying it should be ignored and why.

PerilousApricot commented 8 years ago

If it's truly a tight loop, a simple annotation is all that's needed to silence the warning.

PerilousApricot commented 8 years ago

A compile-time annotation, if it's unclear.

PerilousApricot commented 8 years ago

I've gotten to the point where debug builds work with -Wall -Wextra -Werror -Wno-unused-parameter -Wno-deprecated-declarations -Wno-error=sign-compare, so there's just a few left.