openframeworks / openFrameworks

openFrameworks is a community-developed cross platform toolkit for creative coding in C++.
http://openframeworks.cc
Other
9.94k stars 2.55k forks source link

prevent misinterpretation ofParameter<bool> ctor when single arg is string #8132

Open artificiel opened 1 week ago

artificiel commented 1 week ago

this concerns the constructor of ofParameter, and attempts to sanitize the treatment of single-arg construction such as:

ofParameter<Type> name {some_value};

as described in #7671 currently the implicit conversion will convert string and char * to bool . this is error prone as:

ofParameter<bool> name {"activate shields"};

is not flagged as an error, but creates an unnamed bool param with true as the default value (activate shields implicitly converts to true).

this PR does 2 things:

note that it still allows the exception for void parameters, where the first argument is the name, because there is no value associated with void.

image

this passes the ofParameterGroup/GUI example. closes #7671

(PS: simply marking the constructor explicit was attempted, but that triggers a cascade of errors in ofxGui and friends, which is another can of worms).

roymacdonald commented 1 week ago

I've seen such problem too. This looks like a nice idea. I am just curious about how this might affect implicit conversions from the parameter type into an ofParameter. What if instead we have another single argument constructor with the arg as a string? Probably there will be the need for some sort of disambiguation but might be better in the longer run?

artificiel commented 1 week ago

i've tried the approach of a new constructor enabled for single string or char , but it ended up meaning more cases (the void case, the bool case, the strings/char which is allowed if the ParameterType is string/char *, and other cases which should pass through and let the compiler do it's best to figure out if convertible). but the sore point is it created harder to decipher template errors (something deep in templating implementation instead of a clear "no matching constructor").

adding the is_convertible to the 2-arg version is arguably tightening the requirements, but at the same time, if the arg type is not convertible to the param type, i guess it would not compile anyway? or do something unexpected (like the bool case) and if code relies on that, perhaps it's buggy...? to be frank the documentation of ofParameter is not very generous in regards to the expectations/garantees...!

beyond running ofParameterGroup/GUIexample and some other projects i had currently running, the screenshot above shows a mini test suite to reveal different IDE behaviour for typical usage; perhaps that list could be expanded? do you have more exotic implicit type conversion scenarios to validate? (and the valid cases should be integrated into the ofxUnitTest for ofParameter, which is currently pretty sparse).

roymacdonald commented 1 week ago

I see, what you describe sounds a lot messier. So, what I understand from this is that you can't use the single argument constructor intending it is the name of the parameter, except for the ofParameter. It is intended to be the value, if you want to set the name you need to use the 2 or more arguments constructors. correct?

If so, can you add some inline documentation to the ofParameter class please?

artificiel commented 1 week ago

Yes, the class is very sparsely documented; i've just added docs for the constructors.