Improve C++ standard compliance and reduce pedantic warnings

[OPNA2608]

Related to #216.

BambooTracker has some snippets of code that a compiler set to be very pedantic about standard compliance and good practices will quickly point out and error on. I thought that a meta-issue about fixing them and how to ensure a compliant codebase in the future would be useful.

Some of those problems stem from included projects, I decided to point them out regardless. We could fix them only for ourselves / submit any changes upstream or ignore them for the unforeseeable future.

---

How to verify the problems (or: how to get your compiler to nag about compliance and bad practices):

On GCC/MinGW and Clang:

qmake (the usual options here) QMAKE_CXXFLAGS+="-Wall -Wextra -Werror -pedantic -pedantic-errors (+ whatever exceptions are required to compile, ideally none)"

Or set the QMAKE_CXXFLAGS setting in BambooTracker/BambooTracker.pro I had to set the same setting in BambooTracker/stream/RtAudio/RtAudio.pri and BambooTracker/midi/RtMidi/RtMidi.pri for it to work.
(If you know a better way of setting this, please let me know!)

MSVC appears to have no flag to enable pedantic checking, but it has some variations of /W… ones to enable extra warnings and mark warnings as errors.

Currently needed exceptions required to compile successfully after some easy fixes (tested on Linux, GCC & Clang):

(Prefix to disable a specific warning is -Wno-error=)

• unused-parameter (low priority)
  BambooTracker/chips/c86ctl/c86ctl_wrapper.cpp - several methods
• deprecated-declarations (Clang-only, low prioriy)
  BambooTracker/midi/RtMidi/RtMidi.cpp, several methods - '(method declaration)' is deprecated`
• redundant-move (Clang-only)
  BambooTracker/instrument/instrument.cpp, several methods - redundant move in return statement
• type-limits
BambooTracker/chips/codec/ymb_codec.hpp, line 14 - comparison of unsigned expression < 0 is always false
• maybe-uninitialized
BambooTracker/playback.cpp line 336 - »storeEffectToMap.bool (PlaybackManager::*)(int, Effect)::__pfn« may be uninitialised
BambooTracker/command/pattern/set_effect_value_to_step_command.cpp, line 35 - 'value' may be used uninitialized in this function
• vla
BambooTracker/streams/RtAudio/RtAudio.cpp, line 8279 & 8349 - ISO C++ forbids variable length array 'bufs'

Easy fixes:

(No warranty for any of these being the correct way to resolve the issues.)

Excessive ; in instrument_selection_dialog.hpp.

diff --git a/BambooTracker/gui/instrument_selection_dialog.hpp b/BambooTracker/gui/instrument_selection_dialog.hpp
index 8e84ec7..cc81d29 100644
--- a/BambooTracker/gui/instrument_selection_dialog.hpp
+++ b/BambooTracker/gui/instrument_selection_dialog.hpp
@@ -9,7 +9,7 @@ class AbstractBank;
 
 namespace Ui {
        class InstrumentSelectionDialog;
-};
+}
 
 class InstrumentSelectionDialog : public QDialog
 {

Enumeral and non-enumeral type in conditional expression in pattern_editor_panel.cpp.

diff --git a/BambooTracker/gui/pattern_editor/pattern_editor_panel.cpp b/BambooTracker/gui/pattern_editor/pattern_editor_panel.cpp
index c24343e..8621fd0 100644
--- a/BambooTracker/gui/pattern_editor/pattern_editor_panel.cpp
+++ b/BambooTracker/gui/pattern_editor/pattern_editor_panel.cpp
@@ -110,7 +110,7 @@ PatternEditorPanel::PatternEditorPanel(QWidget *parent)
        setContextMenuPolicy(Qt::CustomContextMenu);
 
        // Shortcuts
-       auto getKeys = [](bool isShift, auto key) { return (isShift ? (Qt::SHIFT + key) : key); };
+       auto getKeys = [](bool isShift, auto key) { return (isShift ? (Qt::Key) (Qt::SHIFT + key) : key); };
        auto jumpLambda = [&] (bool isShift, auto getFunc) {    // For lazy evaluation
                if (bt_->isPlaySong() && bt_->isFollowPlay()) return;
                moveCursorToDown(getFunc());

Comparison of integer expressions of different signedness in command_sequence.cpp:

@@ -186,7 +186,7 @@ int CommandSequence::Iterator::getSequenceType() const
 
 int CommandSequence::Iterator::getCommandType() const
 {
-       return (pos_ == -1 || pos_ >= seq_->getSequenceSize())
+       return (pos_ == -1 || pos_ >= (long int) seq_->getSequenceSize())
                        ? -1
                        : isRelease_
                          ? static_cast<int>(seq_->getSequenceTypeAt(pos_) * relReleaseRatio_)
@@ -195,7 +195,7 @@ int CommandSequence::Iterator::getCommandType() const
 
 int CommandSequence::Iterator::getCommandData() const
 {
-       return (pos_ == -1 || pos_ >= seq_->getSequenceSize()) ? -1 : seq_->getSequenceDataAt(pos_);
+       return (pos_ == -1 || pos_ >= (long int) seq_->getSequenceSize()) ? -1 : seq_->getSequenceDataAt(pos_);
 }
 
 int CommandSequence::Iterator::next(bool isReleaseBegin)

ISO C++ prohibits anonymous structs in bank.cpp: (you probably want to replace vals_type and vals in this with something proper)

diff --git a/BambooTracker/instrument/bank.cpp b/BambooTracker/instrument/bank.cpp
index 9ed1f93..3020e1d 100644
--- a/BambooTracker/instrument/bank.cpp
+++ b/BambooTracker/instrument/bank.cpp
@@ -53,12 +53,12 @@ void WopnBank::WOPNDeleter::operator()(WOPNFile *x)
 struct WopnBank::InstEntry
 {
        WOPNInstrument *inst;
-       struct {
+       struct vals_type {
                bool percussive : 1;
                unsigned msb : 7;
                unsigned lsb : 7;
                unsigned nth : 7;
-       };
+       } vals;
 };
 
 WopnBank::WopnBank(WOPNFile *wopn)
@@ -72,14 +72,14 @@ WopnBank::WopnBank(WOPNFile *wopn)
 
        for (size_t i = 0; i < instMax; ++i) {
                InstEntry ent;
-               ent.percussive = (i / 128) >= numM;
-               WOPNBank &bank = ent.percussive ?
+               ent.vals.percussive = (i / 128) >= numM;
+               WOPNBank &bank = ent.vals.percussive ?
                        wopn->banks_percussive[(i / 128) - numM] :
                        wopn->banks_melodic[i / 128];
-               ent.msb = bank.bank_midi_msb;
-               ent.lsb = bank.bank_midi_lsb;
-               ent.nth = i % 128;
-               ent.inst = &bank.ins[ent.nth];
+               ent.vals.msb = bank.bank_midi_msb;
+               ent.vals.lsb = bank.bank_midi_lsb;
+               ent.vals.nth = i % 128;
+               ent.inst = &bank.ins[ent.vals.nth];
                if ((ent.inst->inst_flags & WOPN_Ins_IsBlank) == 0)
                        entries_.push_back(ent);
        }
@@ -100,7 +100,7 @@ std::string WopnBank::getInstrumentIdentifier(size_t index) const
 {
        const InstEntry &ent = entries_.at(index);
        char identifier[64];
-       sprintf(identifier, "%c%03d:%03d:%03d", "MP"[ent.percussive], ent.msb, ent.lsb, ent.nth);
+       sprintf(identifier, "%c%03d:%03d:%03d", "MP"[ent.vals.percussive], ent.vals.msb, ent.vals.lsb, ent.vals.nth);
        return identifier;
 }

What else?

There are some more warnings, even with all error settings turned up to 100.

• BambooTracker/chips/mame/ymdeltat.c, line 221 - left-hand operand of comma expression has no effect
• some unused-value ones
• On Linux, with Clang and hardening enabled, I get

In file included from /nix/store/7l6vcfqlq5zsxanx93yp6av1r9rny2fk-glibc-2.30-dev/include/string.h:494,
                 from format/wopn_file.c:26:
In function 'strncpy',
    inlined from 'WOPN_writeInstrument' at format/wopn_file.c:186:5:
/nix/store/7l6vcfqlq5zsxanx93yp6av1r9rny2fk-glibc-2.30-dev/include/bits/string_fortified.h:106:10: warning: '__builtin_strncpy' output may be truncated copying 32 bytes from a string of length 33 [-Wstringop-truncation]
  106 |   return __builtin___strncpy_chk (__dest, __src, __len, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Ensuring future compliance

That's where I'm abit lost. The flags aren't compatible across compilers, mostly due to MSVC. I'm thinking if we could detect the compiler & add code to the project file to add the appropriate flags, don't have much time to investigate it right now either.

[rerrahkr]

I fixed errors and warnings until 4fcaeca, but it still remain others. Some of them don't seem to be warned on Windows. I'll try compiling on other OS if I have the opportunity.

[OPNA2608]

Here's a patch to add the warning & pedantic compiler flags (MSVC ones untested, but I read that they might work?) to the project file + fix a pedantic error to make it compile. I've marked the error degradations via a separate line. Compilation succeeds on Linux + hardening w/ GCC, I've attached a build log below that contains the remaining problems that require those no-error flags.

diff --git a/BambooTracker/BambooTracker.pro b/BambooTracker/BambooTracker.pro
index 76d0e82..00a4504 100644
--- a/BambooTracker/BambooTracker.pro
+++ b/BambooTracker/BambooTracker.pro
@@ -40,7 +40,21 @@ else {
 
 CONFIG += c++14
 
-msvc:QMAKE_CXXFLAGS += /source-charset:utf-8
+# C/C++ compiler flags
+msvc {
+  CPPFLAGS += /Wall /Wp64 /WX
+  CPPFLAGS += /source-charset:utf-8
+}
+else:if(gcc|clang) {
+  CPPFLAGS += -Wall -Wextra -Werror -pedantic -pedantic-errors
+  # Temporary known-error downgrades
+  CPPFLAGS += -Wno-error=vla -Wno-error=stringop-truncation -Wno-error=deprecated-declarations
+}
+else {
+  message("Unknown compiler, won't attempt to add warning & pedantic compiler switches.")
+}
+QMAKE_CFLAGS += $$CPPFLAGS
+QMAKE_CXXFLAGS += $$CPPFLAGS
 
 SOURCES += \
     chips/c86ctl/c86ctl_wrapper.cpp \
diff --git a/BambooTracker/stream/RtAudio/RtAudio.cpp b/BambooTracker/stream/RtAudio/RtAudio.cpp
index c16566c..a805c0b 100644
--- a/BambooTracker/stream/RtAudio/RtAudio.cpp
+++ b/BambooTracker/stream/RtAudio/RtAudio.cpp
@@ -2018,7 +2018,7 @@ struct JackHandle {
 };
 
 #if !defined(__RTAUDIO_DEBUG__)
-static void jackSilentError( const char * ) {};
+static void jackSilentError( const char * ) {}
 #endif
 
 RtApiJack :: RtApiJack()

Full build log. (Building part starts on line 364)

[rerrahkr]

-Wno-error=stringop-truncation is added from GCC8, so it should be excluded from CPPFLAGS before that.

In MSVC, /WX generates some errors below:

error C2220: warning treated as error - no 'object' file generated

C2220 is only appeared when the /WX flag is set, so we may need to remeve it (other reports)

and also happened link error on RtAudio with MSVC, it needs to insert -luser32 to the option in RtAudio.pri:

win32 {
    greaterThan(QT_MAJOR_VERSION, 4):greaterThan(QT_MINOR_VERSION, 5) {
        DEFINES += __WINDOWS_WASAPI__
        LIBS += -lole32 -lwinmm -lksuser -lmfplat -lmfuuid -lwmcodecdspuuid
    }
    DEFINES += __WINDOWS_DS__
    LIBS += -lole32 -lwinmm -ldsound -luser32    # Modified
}

After fixing these, it works well on my emvironment. I'll fix and commit.

---

I checked your build log,

stream/RtAudio/RtAudio.cpp:8288:13: warning: ISO C++ forbids variable length array 'bufs' [-Wvla]
 8288 |       void *bufs[channels];
      | 

Is this warning avoidable to replace vla with new void*[channels]?

    else {
      void *bufs = new void*[channels];
      size_t offset = stream_.bufferSize * formatBytes( format );
      for ( int i=0; i<channels; i++ )
        bufs[i] = (void *) (buffer + (i * offset));
      result = snd_pcm_readn( handle[1], bufs, stream_.bufferSize );
      delete[] bufs;
    }

[OPNA2608]

In MSVC, /WX generates some errors below:

That is intentional, to point out code problems right away instead of hiding them in the build log. The -Werror switch on GCC/Clang does the same thing:

   -Werror
       Make all warnings into errors.

I think this is the right way to tackle this issue: Make everything an issue, see where we crash and fix the problem / mask it if it's out of our reach (like system headers, see commit below).

Would you prefer removing that from the other compiler setting as well?

---

Is this warning avoidable to replace vla with new void*[channels]?

It is avoidable with
void **bufs = new void*[channels]; (mind the doubled *)
otherwise several lines complain about void* arithmetic, void* destruction and a method getting void* instead of void**.

-Wno-error=stringop-truncation is added from GCC8, so it should be excluded from CPPFLAGS before that.

OPNA2608@d534a18 is a commit to BambooTracker.pro and the RtMidi/RtAudio files that patches those warnings and adds more sophisticated switch appending via (on Clang slightly hacky) compiler version detection. It should fix the current build issues on this repo's CI. Additionally, I made sure that it also works with the CI targets that would be added in #200.
Build logs:

• https://travis-ci.com/github/OPNA2608/BambooTracker/builds/170773910
• https://ci.appveyor.com/project/OPNA2608/bambootracker/builds/33447438

Locally built binary works fine with these changes, but I'm no C++ guru and it was quickly hacked together so you should prolly take a closer look at it first.

[rerrahkr]

OPNA2608/BambooTracker@d534a18

I checked your commit and it seems works well as far as I can see.

[rerrahkr]

I think this is the right way to tackle this issue: Make everything an issue, see where we crash and fix the problem / mask it if it's out of our reach (like system headers, see commit below).

Would you prefer removing that from the other compiler setting as well?

I also think you should fix all errors as you say.
When I checked the build log again, I noticed that the problem area was recorded. I didn't seem to interpret the meaning of the error well, I'm sorry. I fix an error at fa293b8 and try to resolve others.

[OPNA2608]

I also think you should fix all errors as you say.

Would you like me to open a PR with the leftover changes from that commit?

[rerrahkr]

I also think you should fix all errors as you say.

This is incorrect translation, "I also think we should fix all errors as you say", not only you. I'm sorry to make many mistakes.

Would you like me to open a PR with the leftover changes from that commit?

If possible, I would like you to do. Especially your .pro compilation conditions are more better than mine. Also, merging your PR will record your many contributions and efforts in master history.

[OPNA2608]

With the PR merged, I see the following leftover problems/questions:

1. -Wno-error=stringop-truncation is currently required due to some string truncation in format/wopn_file.c:186. This warning seems to be the easiest to fix as the error indicates it's a simple off-by-one. I had a quick stab at it afew days ago but couldn't do much programming since.

2. The deprecate JACK method actually seems to be a problem on our side. If the JACK headers contain a jack_port_rename method, JACK_HAS_PORT_RENAME should be defined and that method is used. Otherwise, the marked-deprecated jack_port_set_name method is used as a fallback, triggering that build error. Link to upstream's build system showcasing this.
   I'm unsure how to handle this in an "under-the-hood" manner with qmake, but we could amend the compilation instructions to mention that a user should also try passing DEFINES+=JACK_HAS_PORT_RENAME when JACK is requested, dropping that define if it results in an error about jack_port_set_name. Not the most elegant solution, but one more warning off the list.

3. -Wno-error=deprecated-register might have to stay for good. The JACK available on Homebrew is actually JACK1 instead of JACK2, due to packaging problems with the latter. Presumably JACK2 wouldn't have this problem, but until Homebrew figures out a way to package a newer JACK version or we drop JACK1 support altogether (JACK2 on macOS would alternatively be available via the Nix package manager), it will prolly have to stay.

4. We should look into merging any code changes to dependencies that resulted from this compliance issue back into the corresponding projects, if possible & applicable.