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

COMMON: Pass U32String format params by reference and fix warnings about passing a non-trivial class to va_start. #4280

Merged
merged 1 commit into from Sep 24, 2022

Conversation

elasota
Copy link
Contributor

@elasota elasota commented Sep 19, 2022

No description provided.

…out passing a non-trivial class to va_start.
@elasota elasota requested a review from lephilousophe Sep 20, 2022
Copy link
Member

@lephilousophe lephilousophe left a comment

This looks good to me.
As a side note, we are not doing this perfectly as it would require && type for param and the use of std::forward to pass parameters to formatInternal.
As we don't want std function, we could reimplement it ourselves.
But as we use this with good old C data (printf doesn't handle anything else) I think it should work great without all of this.

As a result, this change avoids to call U32String::format with a copy of the format string and removes an implementation-defined behavior (passing a complex class as last argument of a variadic function)

@@ -195,6 +198,11 @@ class U32String : public BaseString<u32char_type_t> {
friend class String;
};

template<class... TParam>
inline U32String U32String::format(const U32String &fmt, TParam... param) {
Copy link
Member

@lephilousophe lephilousophe Sep 20, 2022

Choose a reason for hiding this comment

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

Make use of FORCEINLINE macro here.

@lephilousophe
Copy link
Member

lephilousophe commented Sep 20, 2022

One more note: normally C++ variadic templates are supported by all our toolchains according to the wiki

@sev-
Copy link
Member

sev- commented Sep 24, 2022

looks good

@lephilousophe
Copy link
Member

lephilousophe commented Sep 24, 2022

Let's go!
Thanks

@lephilousophe lephilousophe merged commit 55d31b0 into scummvm:master Sep 24, 2022
8 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
3 participants