Added a null check before checking the flag of an instrument

[SeleDreams]

I added this check because else LMMS would crash if a track had no instrument, when we want to add midi notes, this is unlikely to happen for standard InstrumentTracks but is still important, as it caused crashes during my development of a new type of track for vocal instruments.

[LmmsBot]

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Windows

• Windows 64-bit: lmms-1.3.0-alpha.1.71%2Bg6dde2cd93-mingw-win64.exe (build link)
• Windows 32-bit: lmms-1.3.0-alpha.1.71%2Bg6dde2cd93-mingw-win32.exe (build link)
• Windows 32-bit: lmms-1.3.0-alpha-msvc2017-win32.exe (build link)
• Windows 64-bit: lmms-1.3.0-alpha-msvc2017-win64.exe (build link)

Linux

• Linux (AppImage): lmms-1.3.0-alpha.1.71%2Bg6dde2cd-linux-x86_64.AppImage (build link)

macOS

• macOS : lmms-1.3.0-alpha.1.71%2Bg6dde2cd93-mac10.14.dmg (build link)

🤖

{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://12780-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.71%2Bg6dde2cd93-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12780?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://12776-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.71%2Bg6dde2cd93-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12776?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/01wvoe5kd8uied3t/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38067667"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/hc4lh0y0i0jknqde/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38067667"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://12779-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.71%2Bg6dde2cd-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12779?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://12777-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.71%2Bg6dde2cd93-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12777?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "1940b2aca8bf93aad5c3a360c843414035373299"}

[JohannesLorenz]

LGTM

[IanCaio]

@DomClark No rush to answer as it's something simple to reverse if necessary, but just double checking: I remember you mentioning null-checks for invalid states could potentially hide bugs (i.e.: if an instrument track without an instrument is an invalid state, this hides the issue until it shows itself somewhere else). Is that true here? If so, should we replace the sanity check with an ASSERT?

[DomClark]

Yes, this is one of the places I think an assertion would be better. I'm inclined to leave this as is for the moment, however, and address null safety more generally across the codebase in the future. I think we should annotate when pointers can/cannot be null, either with code or in doxygen comments, and back these up with well-placed assertions. Assuming any pointer can be null and testing before each use is definitely not the way to go.

[JohannesLorenz]

Could we write our own class template, like unique_ptr_not_null<T>?

[DomClark]

That's certainly an option. There are a couple of libraries out there that offer this sort of thing too, such as the GSL and Dropbox's nn. Alternatively we could just use a transparent type alias, such as template<typename T> using NotNull = T;. These all have slightly different semantics, with various advantages and disadvantages, but I think it would be better to discuss these in a dedicated issue or on Discord, rather than at the bottom of an already-merged PR.