overloaded virtual methods in `plugins/LadspaEffect/calf/veal/src/analyzer.cpp` etc. break compilation on newer g++

FyiurAmron commented 4 months ago

System Information

GCC >=13.0

LMMS Version(s)

1.3.0-alpha (master)

Most Recent Working Version

No response

Bug Summary

As per https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87729 , g++ versions >= 13.0 have -Woverloaded-virtual in -Wall now (as visible in https://gcc.gnu.org/bugzilla/buglist.cgi?bug_status=RESOLVED&resolution=FIXED&target_milestone=13.0 ). This, however, comes with caveats ( https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109740 ) - and, in our case, breaks e.g. the compilation of plugins/LadspaEffect/calf/veal/src/analyzer.cpp .

The best solution IMO would be to rewrite the code so that no warning/error will be emitted, i.e. as per official docs: the simplest solution is to add a using-declaration to the derived class to un-hide the base function, e.g. add using A::f; to B. etc. (see https://github.com/yhirose/cpp-peglib/commit/bd0e43e5508cbfeec64955c49d5aa3a848b3fa0b how the fix looks for some code - we already had a similar effort at https://github.com/LMMS/lmms/pull/3215/files etc.)

_{Of course, suppressing it is also an option (although IMVHO with such an easy fix there's no real reason to suppress this).}

BTW, I'd be happy to do a PR for this once this gets accepted/confirmed, the change would be simple enough :D

Note: this has previously surfaced in logs in https://github.com/LMMS/lmms/issues/2044 and https://github.com/LMMS/lmms/issues/3051 as a warning, but since g++ now has it in -Wall, it's no longer a "minor problem" IMO, as it both breaks CI and is a general nuisance ATM. This is, in turn, a blocker for bumping the build CI to bleeding edge versions ATM.

Expected Behaviour

Build should be possible on g++ >= 13.0 with no compilation-breaking warnings/errors.

Steps To Reproduce

get gcc/g++ >= 13.0
try to build
notice the warnings/errors

Logs

compilation log

In file included from /__w/lmms/lmms/plugins/LadspaEffect/calf/veal/src/analyzer.cpp:24:
/__w/lmms/lmms/plugins/LadspaEffect/calf/veal/src/calf/giface.h:782:18: error: 'virtual bool veal_plugins::frequency_response_line_graph::get_layers(int, int, unsigned int&) const' was hidden [-Werror=overloaded-virtual=]
  782 |     virtual bool get_layers(int index, int generation, unsigned int &layers) const;
      |                  ^~~~~~~~~~
In file included from /__w/lmms/lmms/plugins/LadspaEffect/calf/veal/src/analyzer.cpp:25:
/__w/lmms/lmms/plugins/LadspaEffect/calf/veal/src/calf/analyzer.h:74:10: note:   by 'bool veal_plugins::analyzer::get_layers(int, unsigned int&) const'
   74 |     bool get_layers(int generation, unsigned int &layers) const;
      |          ^~~~~~~~~~
/__w/lmms/lmms/plugins/LadspaEffect/calf/veal/src/calf/giface.h:780:18: error: 'virtual bool veal_plugins::frequency_response_line_graph::get_gridline(int, int, int, float&, bool&, std::string&, veal_plugins::cairo_iface*) const' was hidden [-Werror=overloaded-virtual=]
  780 |     virtual bool get_gridline(int index, int subindex, int phase, float &pos, bool &vertical, std::string &legend, cairo_iface *context) const;
      |                  ^~~~~~~~~~~~
/__w/lmms/lmms/plugins/LadspaEffect/calf/veal/src/calf/analyzer.h:73:10: note:   by 'bool veal_plugins::analyzer::get_gridline(int, int, float&, bool&, std::string&, veal_plugins::cairo_iface*) const'
   73 |     bool get_gridline(int subindex, int phase, float &pos, bool &vertical, std::string &legend, cairo_iface *context) const;
      |          ^~~~~~~~~~~~
/__w/lmms/lmms/plugins/LadspaEffect/calf/veal/src/calf/giface.h:217:18: error: 'virtual bool veal_plugins::line_graph_iface::get_moving(int, int, int&, float*, int, int, int&, uint32_t&) const' was hidden [-Werror=overloaded-virtual=]
  217 |     virtual bool get_moving(int index, int subindex, int &direction, float *data, int x, int y, int &offset, uint32_t &color) const { return false; }
      |                  ^~~~~~~~~~
/__w/lmms/lmms/plugins/LadspaEffect/calf/veal/src/calf/analyzer.h:72:10: note:   by 'bool veal_plugins::analyzer::get_moving(int, int&, float*, int, int, int&, uint32_t&) const'
   72 |     bool get_moving(int subindex, int &direction, float *data, int x, int y, int &offset, uint32_t &color) const;
      |          ^~~~~~~~~~
/__w/lmms/lmms/plugins/LadspaEffect/calf/veal/src/calf/giface.h:781:18: error: 'virtual bool veal_plugins::frequency_response_line_graph::get_graph(int, int, int, float*, int, veal_plugins::cairo_iface*, int*) const' was hidden [-Werror=overloaded-virtual=]
  781 |     virtual bool get_graph(int index, int subindex, int phase, float *data, int points, cairo_iface *context, int *mode) const;
      |                  ^~~~~~~~~
/__w/lmms/lmms/plugins/LadspaEffect/calf/veal/src/calf/analyzer.h:71:10: note:   by 'bool veal_plugins::analyzer::get_graph(int, int, float*, int, veal_plugins::cairo_iface*, int*) const'
   71 |     bool get_graph(int subindex, int phase, float *data, int points, cairo_iface *context, int *mode) const;
      |          ^~~~~~~~~
cc1plus: all warnings being treated as errors
gmake[2]: *** [plugins/LadspaEffect/calf/CMakeFiles/veal.dir/build.make:77: plugins/LadspaEffect/calf/CMakeFiles/veal.dir/veal/src/analyzer.cpp.obj] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:6228: plugins/LadspaEffect/calf/CMakeFiles/veal.dir/all] Error 2

Available in organic CI logs at https://github.com/FyiurAmron/lmms/actions/runs/9259008438/job/25470117717#step:9:483

Screenshots / Minimum Reproducible Project

No response

Please search the issue tracker for existing bug reports before submitting your own.

[X] I have searched all existing issues and confirmed that this is not a duplicate.

FyiurAmron commented 4 months ago

Slightly related: https://github.com/LMMS/lmms/pull/7283 CC @Rossmaxx

Rossmaxx commented 4 months ago

Seems like you are touching the calf submodule. In that case, i would request you to upstream the changes to the calf repo too. Ie, open the PRs in LMMS/veal and calf-studio/calf.

FyiurAmron commented 4 months ago

@Rossmaxx Sure thing, but they have it fixed upstream already: https://github.com/calf-studio-gear/calf/blob/master/src/calf/analyzer.h#L61

I've submitted the downstream patch to https://github.com/LMMS/veal/pull/12 ; while it was possible to silence it in this repo, I concur it's better to have it properly fixed at the source.

Rossmaxx commented 4 months ago

We didn't bump the submission to reflect this change.

JohannesLorenz commented 4 months ago

We didn't bump the submission to reflect this change.

@Rossmaxx sorry, what do you mean?

Rossmaxx commented 4 months ago

LMMS is still at the older commit. A PR similar to #6771 is needed right?

JohannesLorenz commented 4 months ago

LMMS is still at the older commit. A PR similar to #6771 is needed right?

Yes, correct.

FyiurAmron commented 4 months ago

@Rossmaxx @JohannesLorenz would something like https://github.com/LMMS/lmms/pull/7290 suffice? I'm no expert at git submodules, but git submodule update --remote --merge should be enough in this case, right?

JohannesLorenz commented 4 months ago

Interesting point with the --remote, you never stop learning here :+1: Imo that should be correct.

LMMS / lmms