This indicates that the code in question is not covered by tests. I work on a database, for which correctness is paramount, so it's unnerving to see any code fixes not associated with test additions or changes. I would have thought that cryptocurrency had similar standards.
We've modified the dlang Github tester to run coverage tests over Pull Requests, and uncovered changes get marked with a scarlet bar. While some changes can't be tested with a test suite, the result has been a marked improvement in the quality of pull requests (including mine) because it is embarrassing to submit untested code.
It should be an error, not a warning (default or not). There are a number of things like this that should just not be accepted anymore. Here's another one:
if (condition);
dothis();
No compiler should accept that, regardless of switch settings.
It should be an error, not a warning (default or not).
In C++ you can overload '==' to do anything you want, including things that have side effects[0]. Without deeper introspection into the code, it's tricky to check if it's an error or not.
-Wall sure (also, it should really enable all warnings, just as the name implies. Currently it doesn't)
-Werror, I'm not sure. During development and on CI systems it makes sense. But if you intend to ship source code, it's probably best left off. Other people might be using different compilers that emit warnings for things yours didn't. You don't want the build to fail for those people.
> Other people might be using different compilers that emit warnings for things yours didn't. You don't want the build to fail for those people.
Actually, you do. That way those things will be fixed and hopefully your inclusion of their fixes is a well motivated PR away.
Switching -Werror off should be a decision made with great care and understanding of what's going on under the hood. If there are platform specific issues then people on those platforms will be the ones in the best position to determine if such a warning is actually an error or not.
I want to agree with you but in practice I can't really imagine switching -Werror outside of dev builds. The problem is that different compilers have different warnings and if your code is meant to be portable it's going to be a serious pain to avoid all the warnings all the time.
Having compilation break for a user because of a spurious warning due to a different compiler (or different compiler version) would make it a lot more painful to maintain.
For instance I started maintaining an old C++ codebase that generates a bunch of warnings when build with a modern g++, code like this:
#define M "toto"
const char *s = "tata"M;
Generates warnings like:
warning: invalid suffix on literal; C++11 requires a space between literal and string macro [-Wliteral-suffix]
Is it helpful? Sure. It is worth breaking the build for anybody using a modern g++ until I manage to fix all occurrences and release a new version? I don't think so.
An other example is building code with -Wextra which warns you when some comparisons become "nops". For instance:
int toto(int i) {
if (i <= 0xffff)
return 1;
return 0;
}
On most modern machines this will build without a hitch, but if you compile on a 16bit machine you'll get a warning saying:
warning: comparison is always true due to limited range of data type
In my experience this particular warning is generally not a problem. On the other hand the following code:
int i = 0xabcdef;
Will generate the following warning on 16bit architectures:
warning: integer constant is too large for its type
And that's potentially a lot nastier and should be an error IMO.
So I'd say the main problem is that C and C++ are way too permissive by default, things like comparison with no side effects ought to be errors, not warnings. And there are a bunch of others, for instance why on earth are these not errors :
warning: no return statement in function returning non-void
warning: ‘i’ is used uninitialized in this function
So -Werror is just killing a fly with a bazooka IMO.
warning: comparison is always true due to limited range of data type
Surely this is a serious problem on 16 bit builds; the comparison implies that larger values are expected to be stored in this variable, the warning is a strong hint that the (size of the) types are incorrect.
In my experience it's generally not the case, you generally do these tests if you're about to truncate a value or do sanity checks. If the value is too big to fit the variable then it should've been caught earlier, when the value was loaded in the first place (i.e. strtol or whatever).
It gets worse for this particular project. They do build with -Wall and -Wextra, but they ignore many warnings that the compiler does emit, and a lot of them are adding/removing a single word. Also, compilers have different default warnings. Gcc doesn't emit a warning on that code, but clang does, for example.
Our user base is steadily growing. Coming soon is a nice upgrade to the language designed to eliminate stray pointers into expired stack frames.
I feel it is my duty as a language designer to make it hard for programmers to commit common bugs. It's just too expensive for companies to ship these kinds of things.
Any C++ static analyzer worth its salt would catch this. I don't think this is so much of a C++ language issue, but more of an issue with C++ environments/compilers not doing static analyzing by default, and/or the authors of this software not being aware of the fact that they need to use static analyzers.
You're right. My contention is that the language itself should no longer accept such code, it shouldn't be relegated to an ignorable warning or a 3rd party tool that isn't run.
Indeed, the next standard should ban such things, and modern compilers should reject these patterns by default, unless a --legacy_security_unsage flag is set.
so nobody is going to type that in by accident, and nobody is going to want to defend it in the code review process. And the compiler vendor will still get a gold star for being 100% language compliant :-)
Yes, major improvements could be made by changing the defaults of the tools that beginners use. I edited my original comment to be more clear on this as well.
I think we're going to have to disagree. It isn't just an issue for beginner code. Experts make such mistakes, too. Expecting them to not make mistakes is unrealistic, and sending them back for more training isn't going to work, either.
I can't think of a reason any modern language should accept such code.
If you're interested, I recommend the series "Air Disasters" on TV. Each episode analyzes one crash, where investigators determine the cause of the accident, and what the corrective action should be taken. Often, it comes down to "how could a pilot with 10,000 hours experience make such a mistake". While it would be easy for investigators to blame the pilot, often they find that the mistake was just too easy to make, and they recommend changing the design or procedures or both.
It gets more fun when you're dealing with things like memory mapped I/O, where reading from a register is a destructive action, so even a non-overloaded == can cause side effects.
https://github.com/zcoinofficial/zcoin/blob/81a667867b5d8489...
and the line of code:
In other words, a statement with no effect. In D, such a line gives an error, not a warning: You can force the statement to be accepted by casting it to void.It's really past time for languages to not accept such code any more. Legacy code needs to get fixed.