Which warnings are worth fixing?

[malespiaut]

Hello,

I usually compile my projects with the list of warnings:

-Wall
-Wextra
-Wpedantic
-Wshadow
-Wreturn-type // Enabled by -Wall in C++
-Wint-conversion // C-only warning
-Wstrict-aliasing=2
-Wdouble-promotion

and I've complied the latest revision of MilkyTracker using these.

As a result, when sorting and cleaning compiler output, it turns out that there are 1481 unique warnings for MilkTracker.

Here's a list of the number of occurence:

  1 -Waddress
  8 -Wclass-memaccess
 10 -Wdelete-non-virtual-dtor
  2 -Wdeprecated-copy
 71 -Wdouble-promotion
  2 -Wextra
  7 -Wformat=
  1 -Wformat-extra-args
  1 -Wformat-overflow=
 10 -Wimplicit-fallthrough=
  1 -Wint-in-bool-context
  6 -Wmisleading-indentation
  1 -Woverloaded-virtual=
  4 -Wparentheses
 44 -Wreorder
  1 -Wrestrict
980 -Wshadow
 55 -Wsign-compare
  1 -Wtype-limits
  4 -Wunknown-pragmas
 13 -Wunused-but-set-variable
  3 -Wunused-function
213 -Wunused-parameter
 41 -Wunused-variable

There's also an extra PPSystem_POSIX.cpp:(.text+0x13): warning: the use of tmpnam' is dangerous, better use mkstemp'.

Luckily, a lot of them are easily fixable. -Wunused-parameters are only a matter of adding (void)name_of_unused_parameter in the function, and -Wshadow is “only” about renaming variables.

While I wanted to commit to your project hoping to increase code quality, I don't know if you are looking forward to review, test, and merge such commits. Hence this message.

Best regards

[coderofsalvation]

Hi malespiaut, so far I guess warnings ares a bit of an 'it aint hurt until it hurts' kind of thing.
My thoughts are:

• TBH I'm not really sure how much refactoring it would trigger.
• Most dev for next release is happening in src/tracker and src/ppui, perhaps the other folders
• are you able to test crossplatform? (Mac/win/Linux flavors?)

[coderofsalvation]

btw forgot to mention: thank you very much for the heads up.
Perhaps there's a bunch of warnings which don't require crossplatform testing (the current CI builds windows & linux) but I don't know much about mac.

btw2. which category warnings are about functions not returning a value? We got bitten by that in the past :)

[malespiaut]

Thank you for your answer @coderofsalvation

I, for one, wouldn't go down the path of refactoring the code to fix warnings. I was thinking about “easy” warnings frist (-Wunused-parameter can be fixed with a simple (void)). I'm too foreign to this codebase to engage on more complex fixes.

I'm not able to test on anything else than Linux for now. But I have an old late-2009 MacBook sleeping around. I think that it can be upgraded from macOS 10.5 to 10.11. One of my friends can help me up with anything macOS-related. But don't hold your breath as I'm, as everyone else here, very busy already. :P