MaslowCNC / Firmware

Maslow Firmware
GNU General Public License v3.0
263 stars 134 forks source link

gcc warnings #412

Closed aprospero closed 6 years ago

aprospero commented 6 years ago

Abstract

Many Arduino programs aren't very complex and have no requirements regarding code maintenance. Consequently the Arduino IDE doesn't make much hassle about compiler warnings. In fact it suppresses those by default. Nevertheless, keeping an eye on compiler warnings have been proven to be good coding habit if it comes to more complex programs - which IMHO Maslow firmware is.

Reason

If a codebase produces too many warnings it gets more likely that possibly important warnings could be overlooked. This pull request aims to clean up the warning logs so that obvious coding problems can be easily detected.

Solution

The main warning classes were unsigned/signed comparisons without cast, unused local variables, unused function parameters and missing function return statements. Most of these are simple no-brainers. When data types were changed, the runtime value range was taken into account. Also return values were checked for consistency before adding them to the code.

What's next

It is obvious that other warnings will pop up again if future contributers are not aware of the issue. This is maybe a too optimistic wish but I'd like to encourage everyone to deal with the gcc compiler and its output. Therefore we have to make it accessible in the first place.

How to activate gcc Output

To activate warning output Arduino IDE users need to activate compiler output in the preferences dialog. Additionally Arduino IDE needs to tell gcc to generate warning messages. This is done in the Arduino IDE's preferences.txt file - you can get there via the Preferences Dialog, at the bottom is a link to the file. Edit the line "compiler.warning_level=none" to "compiler.warning_level=all"

BarbourSmith commented 6 years ago

This is a gorgeous pull request. Well done.

From reading through the code it looks like all of the changes are well executed and the type of things that keeps the repository clean and maintainable.

I agree that encouraging at least the development community to display gcc compiler warnings is a good idea and I will turn them on. What about a comment at the top of the .ino file? That way the text will be visible whenever the IDE is open.

aprospero commented 6 years ago

To put a comment at the top of the .ino is a good idea! I already asked myself how awareness could be raised since pull requests aren't always contributers everyday lecture :). I'll add another commit with the comment.

BarbourSmith commented 6 years ago

One more option could be to check for compiler warnings in the travis-ci tests and that way the tests could tell us if we had anything uncaught...or maybe that's going too far.

I've got my warnings turned on now and everything tests smoothly. 👍 👍

Again, what a beautiful pull request. Thank you.