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

Revert part of #676 because there is no implicit conversion to std::string #680

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

jmcarcell
Copy link
Member

BEGINRELEASENOTES

ENDRELEASENOTES

There are several parts in the stack that rely on implicit conversion (such as from G4STring in k4geo, for example) and #676 disallows that. Alternatively we could keep the static_assert and add another template for anything that can be converted to string but I think the original implementation with the EnableIf is just cleaner. With the getters it should be fine since we are not going to be doing weird conversions (I hope) so the rest of the PR doesn't need to be reversed.

…o std::string

when trying to put a parameter that can be converted to std::string, therefore
it will fail with the static_assert when compiling.
Comment on lines +249 to +250
inline void putParameter(const std::string& key, std::string value) {
putParameter<std::string>(key, std::move(value));
Copy link
Collaborator

@tmadlener tmadlener Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just having the std::string and const char* overloads are not enough for catching all implicit conversions? I.e. bring back the std::string overload.

Copy link
Contributor

@m-fila m-fila Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without SFINAE we get an exact match with a template, only later the conversions would be considered, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, without SFINAE the template will always be preferred when the type is not exactly const char* nor std::string, so having both overloads is not enough.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right. Overload resolution is fun ;)

@tmadlener
Copy link
Collaborator

Also just to make sure: We have the same static_assert in the GenericParameters that actually do the heavy lifting, but we never hit those because all the necessary implicit conversions already happen at the Frame boundary, right?

@jmcarcell
Copy link
Member Author

jmcarcell commented Sep 18, 2024

Yes, it's impossible to hit those when using frames since the ones from the Frame would happen earlier. Maybe it's a bit redundant but then whether it's from a Frame or from the parameters themselves there will be an error.

Actually, having a look at it the GenericParameters have the same problem, if one would want to set them with something that can convert to std::string but it is not the templated overload will be used. I think in this case, since it is less likely that they are used directly this is less of a problem, but we could also revert it the same way it was done here.

@tmadlener tmadlener merged commit 1af6557 into AIDASoft:master Sep 18, 2024
18 checks passed
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.

3 participants