DISTRHO / Cardinal

Virtual modular synthesizer plugin
https://cardinal.kx.studio/
GNU General Public License v3.0
2.28k stars 155 forks source link

update nonlinearcircuits; needs better fix for max macro #586

Closed dromer closed 1 year ago

dromer commented 1 year ago

Builds, but not pretty "fix" for that macro definition.

Adds 5 new modules.

Problem without the macro is:

nonlinearcircuits/src/NLCShared.hpp: In function ‘float SlothLedBrightness(float)’:
MockbaModular/src/MockbaModular.hpp:32:37: error: expected unqualified-id before ‘(’ token
   32 |         #define max(a,b)            (((a) > (b)) ? (a) : (b))
      |                                     ^
nonlinearcircuits/src/NLCShared.hpp:125:17: note: in expansion of macro ‘max’
  125 |     return std::max(0.0f, std::min(1.0f, v / 2.0f));
      |                 ^~~
cosinekitty commented 1 year ago

@dromer I'm glad to see someone is bringing the NLC Sloth modules into Cardinal! As their author, please let me know if I can do anything to help. Hopefully it is a simple process.

falkTX commented 1 year ago

the use of the max macro is not needed, and in fact unwanted as it breaks std::max usage. Rack/Cardinal is C++ so the std::max function is available to use, please use that instead of C-style macros.

cosinekitty commented 1 year ago

Builds, but not pretty "fix" for that macro definition.

Adds 5 new modules.

Problem without the macro is:

nonlinearcircuits/src/NLCShared.hpp: In function ‘float SlothLedBrightness(float)’:
MockbaModular/src/MockbaModular.hpp:32:37: error: expected unqualified-id before ‘(’ token
   32 |         #define max(a,b)            (((a) > (b)) ? (a) : (b))
      |                                     ^
nonlinearcircuits/src/NLCShared.hpp:125:17: note: in expansion of macro ‘max’
  125 |     return std::max(0.0f, std::min(1.0f, v / 2.0f));
      |                 ^~~

Would it help if I updated these to use if statements instead of min and max? Michael Hetrick gave me commit rights to the repo. Then you could update the submodule and these warnings would be fixed.

falkTX commented 1 year ago

hmm actually the issue is on the MockbaModular modules, that is where the macro comes from. so we should modify that part of the code. the NLC modules seem fine

dromer commented 1 year ago

Yeah the issue is NOT with NLC but the conflict is introduced by Mockba over here: https://github.com/MockbaTheBorg/MockbaModular/blob/479d2c8007b2087cdf557a491df25c5b85784a96/src/MockbaModular.hpp#L31-L33

It just arises in NLC because its build is handled alphabetically and comes after Mockba ;)

falkTX commented 1 year ago

@dromer lets go for this, just one request: instead of "define max max" use "#undef max" right after the mockba includes, there is already a "#undef min" so just add it above this line. thanks

dromer commented 1 year ago

Hah yes indeed, totally missed that one :D

falkTX commented 1 year ago

thanks again!