Compile error

[ZonderP]

I'm getting this

C:/mingw64/bin/g++.exe -I./lib -I./src -D TEST_MAX_ARRAY_PRINT=20 -I./tests -g -D JS80P_VERSION_STR=dev -D JS80P_VERSION_INT=999000 -Wall -Werror -msse2 -ffast-math -O3 -o build/x86_64-w64-mingw32/test_gui.exe tests/test_gui.cpp
In file included from tests/gui_stubs.cpp:24,
                 from tests/test_gui.cpp:29:
./src/gui/gui.hpp:277:29: error: 'virtual JS80P::WidgetBase* JS80P::WidgetBase::own(JS80P::WidgetBase*)' was hidden [-Werror=overloaded-virtual=]
  277 |         virtual WidgetBase* own(WidgetBase* widget);
      |                             ^~~
In file included from tests/gui_stubs.cpp:107:
./src/gui/widgets.hpp:125:22: note:   by 'JS80P::ParamEditor* JS80P::TabBody::own(JS80P::ParamEditor*)'
  125 |         ParamEditor* own(ParamEditor* param_editor);
      |                      ^~~

when I try to build latest main branch with '\build.bat' on Windows.

g++ --version gives me this:

g++.exe (MinGW-W64 x86_64-msvcrt-mcf-seh, built by Brecht Sanders) 13.1.0
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Any ideas?

(I'm trying to add some FST ;-) features - like support for bank of programs, program name(s), switching of program etc. as well as support for parameter automation in a host...)

[attilammagyar]

Thanks for checking out the project!

The compiler is right, TabBody shouldn't hide the inherited own() method. I'm pushing a fix in a moment.

As for parameter automation: it was kind of an intentional decision not to expose parameters to the DAW. If they were just exposed normally, then DAW-based automation would conflict with the plugin's internal controller assignments, so it should be decided which kind of automation would win in that case. This question could be eliminated by introducing a special controller for the DAW, because then the automation source for parameters would always be unambiguous. But on a second thought, this seems kind of unnecessary, since this is essentially the same as automating any MIDI CC in the DAW, and then assigning that MIDI CC to any param within the synth, which is already doable without having to write a single line of code.

However, the VST 3 version has an advantage in this area, since that is required by the spec to expose parameters for each MIDI CC, which has side effects that would be nice to have for the FST version as well. I created issue #7 about this, thanks for the idea!

[ZonderP]

Thanks for the quick fix!

The GUI test project now builds successfully.

But now there is a next issue:

C:/mingw64/bin/g++.exe \
        -IC:/mingw64/lib/gcc/x86_64-w64-mingw32/12.2.0/include -IC:/mingw64/include/c++/12.2.0/backward -IC:/mingw64/include/c++/12.2.0/x86_64-w64-mingw32 -IC:/mingw64/include/c++/12.2.0 -IC:/mingw64/include -I./lib/vst3sdk -I./lib -I./src -DRELEASE -DJS80P_VST3_GUI_PLATFORM=kPlatformTypeHWND -D JS80P_VERSION_STR=dev -D JS80P_VERSION_INT=999000 -Wall -Werror -msse2 -ffast-math -O3 -Wno-class-memac
cess -Wno-format -Wno-multichar -Wno-parentheses -Wno-pragmas -Wno-unknown-pragmas -mwindows -D UNICODE -D _UNICODE \
         -c src/plugin/vst3/plugin.cpp -o build/x86_64-w64-mingw32/vst3-plugin-64bit.o
src/plugin/vst3/plugin.cpp: In member function 'std::string JS80P::Vst3Plugin::Processor::read_stream(Steinberg::IBStream*, JS80P::Integer) const':
src/plugin/vst3/plugin.cpp:526:15: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
  526 |     buffer[i] = '\x00';
      |     ~~~~~~~~~~^~~~~~~~
src/plugin/vst3/plugin.cpp:507:37: note: at offset -1 into destination object of size [0, 9223372036854775807] allocated by 'operator new []'
  507 |     char* buffer = new char[max_size];
      |                                     ^
cc1plus.exe: all warnings being treated as errors
mingw32-make: *** [makefile:355: build/x86_64-w64-mingw32/vst3-plugin-64bit.o] Error 1

This one I do not really understand when looking at the code around line 526 in vst3/plugin.cpp - for me it looks right...

Do you use a different build mechanism? Or just an older version of MinGW?
Should I open another issue for the above? (I'm new to open source habits, but know Git from work.)

May I ask how you debug? (I'm used to Visual Studio projects for Windows and lately also learned a little bit about cmake and I possibly should also be able to use Visual Code for this purpose - but I do not yet have any experience with MinGW and makefile).

Would there be another way to communicate with you?

[attilammagyar]

I think the compiler's problem is not that this method does write into a zero sized buffer at the moment, but that it could be called in a way which would result in doing that.

I was in a hurry in the morning, and I closed the issue with only running the test that failed to compile, without checking if everything else is okay. I'm going to fix this some time after work.

May I ask how you debug?

I'm not an experienced C++ developer, so most of the time I rely on the good old fprintf()-based approach. I sometimes use the debug logging macros in src/debug.hpp. When something requires real debugging, especially in the synth engine, I create a small unit test which triggers the issue and then run that with GDB to see what's going on. When the issue is GUI related, there are little GUI playground apps for Windows and Linux in the src/gui/ directory which do nothing but show a window with the GUI, letting you run it in isolation. (Compile them with make guiplayground, they will be built inside the build folder.) And if everything else fails, I place one of the plugins in my VST directory, and load up Reaper in gdb.

Would there be another way to communicate with you?

GH issues is fine for me - the older I get, the more I'm avoiding instant communication platforms, social media, notifications, etc. :-D

[ZonderP]

HeHe, I fixed this temporarily exactly the same way as you did , except that I didn't rename the function.

I'd have 2 suggestions for the Serializer:

1. It would be much cleaner to use Synth& instead of Synth* for public functions 'serialize' and 'import' (and also private functions), because pointers 'could' be nullptr, whereas references cannot.
   (This is also the same for quite some other classes like e.g. ImportPatchButton and ExportPatchButton)
2. passing std::string by value is not good practice, because of bad performance, because the strings get copied unnecessarily.
   Either use 'std::string_view' or 'const std:string&' (the first one should be preferred, but is a bit more work to change, because to access the string one needs to use member function 'data()'.)

I think this passing of strings by value also occurs in other classes...

[ZonderP]

Oh, and the excessive use of 'const' for variables passed in as values also doesn't make too much sense in C++; It's no additional guarantee for the caller of the function - it only means that within the function the value of the variable cannot be changed...
For sure const is very important when values are passed by reference or pointer.

[attilammagyar]

1. I agree, it could and should be done, I'll look into it later. While developing the VST 3 plugin, while I was still trying to wrap my head around it, there were stages where it seemed that the synth might have to be initialized after the GUI is already built, and thus, it would need be null in the objects you mentioned. Now that I got VST 3's challenges sorted out, changes like 98f4354c681073a899b97a04bad890779ca27e6f could be undone.

2. Do they actually get copied though? My impression was that this is what move constructors and copy elision were supposed to prevent, though I admit I didn't actually confirm this assumption. I'll take a look at it later.

3. Some people say that such unnecessary consts just make the code noisy, others suggest that there are cases where it might even help the optimizer, so it looks like its use or omission somewhat boils down to personal taste.

[ZonderP]

@2: I'm quite sure that in most cases passing a string, that should itself not be modified within called functions, by const reference should be the preferred way, since a reference is (usually) just implemented as a pointer. No copying/moving at all.
From my understanding, moving could only be applied for temporary variables, e.g. when you call a function taking a string as parameter like so: foo("My string").
Might be my knowledge about this is out-dated, but I cannot imaging how possibly moving back and forth could be better than just using something like a pointer...
@3: Yeah, const in this scenario does for sure not hurt.

I'm almost done with at least a draft which implements program banks, switching of programs within a bank, naming programs and save and load native fst banks and programs.

[attilammagyar]

I'm almost done with at least a draft which implements program banks, switching of programs within a bank, naming programs and save and load native fst banks and programs.

Thank you, I'll take a look at them, but it may some time, because currently I'm also working on a couple of cool features.

In the meanwhile, I created issue #8 and #9 from your code quality suggestions, so that I won't forget to address them.

[ZonderP]

What would be the procedure to give you my changes for review?
I cloned your repo and created a local branch 'FST-PresetHandling' and so far kept it up to date with main by merging your changes in.

[ZonderP]

Nevermind!
Found a way by using a fork and creating a pull request.
#11

[attilammagyar]

Sorry for being so late; a PR is exactly what I'd have suggested if I'd got to reply sooner. :-)