Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

artificiel
Copy link
Contributor

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:

  • ensures the arg is convertible to the type (better error message, otherwise they come deeper in the template stuff)
  • if the type is bool, ensure arg is also arithmetic (so implicit conversion from 0, 1, or whatever arithmetic value is allowed)

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ambiguity about ofParameter<bool> constructor
4 participants