Most linters and editors would highlight this. Also, if one writes test, one of the test cases should fail.
This is just a typo, not really a quality ad for some code quality monitor.
Everybody makes mistakes when writing comparison functions
11–20 of 63 posts
Re: Everybody makes mistakes when writing comparison functions
#12I have a friend that used to be an OpenSSL contributor (not sure if he still is). Not really a big deal. There were a lot of them.
He was generally cranky ["curmudgeonly"? -Ed.] about the code, and tried submitting quality fixes, where possible; but this was years ago.
PVS-Studio looks great. I wish it worked on Swift. SwiftLint has its limitations.
Re: Everybody makes mistakes when writing comparison functions
#13The author's article about comparison functions linked in the blog post is worth the read Regarding the OpenSSL example presented here, wouldn't any decent IDE catch an unused parameter in a function? Why is a separate static analyzer necessary for this
A lot of people don’t use IDEs to write C.
That said, I believe the two mainstream C compilers (GCC and Clang) can both be configured to emit a warning here.
Re: Everybody makes mistakes when writing comparison functions
#14The author's article about comparison functions linked in the blog post is worth the read Regarding the OpenSSL example presented here, wouldn't any decent IDE catch an unused parameter in a function? Why is a separate static analyzer necessary for this
> Why is a separate static analyzer necessary for this Because this is a nail this co-founder of PVS-Studio saw while holding the hammer he wants to sell. By the way, it seems to me this error does not have much to do with comparison functions, it is a mistake that can be made in many kinds of functions. Unused parameters are warned against in C (hence all the UNUSED macro hacks) and no static analyzers are going to…
I think comparison functions are a prime "use case" for this kind of error because it will pass the type check easily and comparison is often somewhat symmetric, so you will do a lot of copy and paste. Only unit or property tests could catch this (and a compiler warning, of course). The example here is of course extreme, but I bet you will find something like this quite often, especially in OO code like Java.
Re: Everybody makes mistakes when writing comparison functions
#15Unused function argument is a warning any sane compiler can spit out and turn into a hard error (so this is not really the best example of what PVS studio can do for you), which makes one wonder: why was this not caught earlier? Warnings not enabled, or ignored, and is that something which is problematic wrt something as major as OpenSSL?
I also wonder this. "Unused parameter in function" is one of the most basic warning/linting checks. I'd be interested in finding out how this managed to reach release.
Both GCC and clang warn about unused parameters, but only when you run them with -Wextra.
Re: Everybody makes mistakes when writing comparison functions
#16I have a friend that used to be an OpenSSL contributor (not sure if he still is). Not really a big deal. There were a lot of them. He was generally cranky ["curmudgeonly"? -Ed.] about the code, and tried submitting quality fixes, where possible; but this was years ago. PVS-Studio looks great. I wish it worked on Swift. SwiftLint has its limitations.
I was surprised to read in the article that the code quality was good, unless they some how pulled off a major refactor since LibreSSL basically called the code a mess.
If I recall, the claim was that OpenSSL prioritized issues paid for by companies, and the alot was left to rot. I'm curious if your friend saw the same things?
Re: Everybody makes mistakes when writing comparison functions
#17The author's article about comparison functions linked in the blog post is worth the read Regarding the OpenSSL example presented here, wouldn't any decent IDE catch an unused parameter in a function? Why is a separate static analyzer necessary for this
> Why is a separate static analyzer necessary for this Because this is a nail this co-founder of PVS-Studio saw while holding the hammer he wants to sell. By the way, it seems to me this error does not have much to do with comparison functions, it is a mistake that can be made in many kinds of functions. Unused parameters are warned against in C (hence all the UNUSED macro hacks) and no static analyzers are going to…
Last time OpenSSL was in the news for a bug that should generate a warning I took a cursory look at some of the code, it was stuffed with stuff that would generate similar warnings. So at that time I concluded that going through all the warnings would be a pretty big job, and that is likely why nobody did so. On the more speculative front, it might be that some maintainer is against taking whitespace patches, that would probably make anyone who tries give up pretty quickly.
I'm a bit skeptical of the claim "The code quality is excellent", if it was this would be a lone warning emitted by the compiler, and surely it would have been fixed then.
Re: Everybody makes mistakes when writing comparison functions
#18I have a friend that used to be an OpenSSL contributor (not sure if he still is). Not really a big deal. There were a lot of them. He was generally cranky ["curmudgeonly"? -Ed.] about the code, and tried submitting quality fixes, where possible; but this was years ago. PVS-Studio looks great. I wish it worked on Swift. SwiftLint has its limitations.
I was surprised to read in the article that the code quality was good, unless they some how pulled off a major refactor since LibreSSL basically called the code a mess. If I recall, the claim was that OpenSSL prioritized issues paid for by companies, and the alot was left to rot. I'm curious if your friend saw the same things?
There is a difference in when a tool considers code to be of high quality and when humans do.
Re: Everybody makes mistakes when writing comparison functions
#19The author's article about comparison functions linked in the blog post is worth the read Regarding the OpenSSL example presented here, wouldn't any decent IDE catch an unused parameter in a function? Why is a separate static analyzer necessary for this
I think the point is if you integrate the analyzer in the workflow its hard to ignore. The ide warnings are easy to miss sometimes.
The Right Thing™ for a project that actually has multiple contributors is that the CI pipeline rejects code that based on the compiler diagnostics is either definitely wrong or, if it was correct, should be annotated in a way the compiler can understand and stop emitting the diagnostic. It doesn't matter whether one or another contributor has this switched off or ignored it.
That is, committing this code should have had the same impact as if it was missing the semi-colon, the library doesn't build, tree is on fire, fix before you do new work.
Of course if your code is in sufficiently bad state, you might find it's frustrating to have say 500 bugs to solve before you can get the CI pipeline to output artefacts. I think that means you didn't have good software and you must fix those bugs first, but if you're convinced the software is good it can be tempting to instead disable the diagnostics telling you otherwise and press on.
You have 178 seconds to live:
Re: Everybody makes mistakes when writing comparison functions
#20I have a friend that used to be an OpenSSL contributor (not sure if he still is). Not really a big deal. There were a lot of them. He was generally cranky ["curmudgeonly"? -Ed.] about the code, and tried submitting quality fixes, where possible; but this was years ago. PVS-Studio looks great. I wish it worked on Swift. SwiftLint has its limitations.
I was surprised to read in the article that the code quality was good, unless they some how pulled off a major refactor since LibreSSL basically called the code a mess. If I recall, the claim was that OpenSSL prioritized issues paid for by companies, and the alot was left to rot. I'm curious if your friend saw the same things?
I have never gazed upon the code, myself, so only have thirdhand anecdotal information.
He didn't speculate as to "why," but he said the project was very buggy, and rather "messy."
To be fair, we worked for a company (he was one of my employees) that was anal about Quality, and hard to please. It rubbed off on us.