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

Initializer list constructors cause ambiguous overloads in application code #24093

Open
onitake opened this issue Nov 26, 2023 · 3 comments
Open

Comments

@onitake
Copy link

onitake commented Nov 26, 2023

Description

Bug description:

In wxWidgets 3.2.4, support for C++ initializer lists in certain type constructors was introduced. (PR: #23309 )
This causes problems in 3rd party code, where transitive constructors are overloaded with other types that take initializer lists.

Specifically, PrusaSlicer fails to build with 3.2.4.

Expected vs observed behaviour:

Code like this should compile:

#include <string>
#include <vector>
#include <wx/arrstr.h>
struct Test {
	wxArrayString arr;
	Test(const std::vector<std::string> &values) {
		if (values.empty()) return;
		for (const auto &el : values) arr.Add(wxString(el));
	};
	Test(const wxArrayString &values) {
		if (values.empty()) return;
		for (const auto &el : values) arr.Add(wxString(el));
	};
};
int main() {
	Test({ std::string("Hello, world") });
}

This compiled fine with wxWidgets 3.2.3, but with 3.2.4, g++ reports the following error (gcc 13.2.0):

test.cpp: In function ‘int main()’:
test.cpp:16:45: error: call of overloaded ‘Test(<brace-enclosed initializer list>)’ is ambiguous
   16 |         Test({ std::string("Hello, world") });
      |                                             ^
test.cpp:10:9: note: candidate: ‘Test::Test(const wxArrayString&)’
   10 |         Test(const wxArrayString &values) {
      |         ^~~~
test.cpp:6:9: note: candidate: ‘Test::Test(const std::vector<std::__cxx11::basic_string<char> >&)’
    6 |         Test(const std::vector<std::string> &values) {
      |         ^~~~

Potential fixes:

Curiously, I expected that making the initializer list constructors explicit would help, but the transitive overload is still considered ambiguous by gcc:

class WXDLLIMPEXP_BASE wxArrayString : public wxArrayStringBase {
...
	template<typename U>
	explicit wxArrayString(std::initializer_list<U> list) : wxArrayStringBase(list) { }
...
};

Platform and version information

  • wxWidgets version you use: 3.2.4
  • wxWidgets port you use: wxGTK
  • OS and its version: Debian sid
    • GTK version: 3.24.38
    • Which GDK backend is used: X11
    • Desktop environment : N/A
    • Current theme: N/A
@vadz
Copy link
Contributor

vadz commented Nov 26, 2023

Yes, I'm afraid this change shouldn't have been backported as it broke a lot of code and it could have been foreseen.

Unfortunately now that it is, we're not going to remove it as it would make the matters even worse, so the only fix is to work around it in the application code by using the explicit conversion from the initializer list to whichever class you want to use. E.g. in your case you'd use

Test(wxArrayString({ std::string("Hello, world") }));

or

Test(std::vector<std::string>({ std::string("Hello, world") }));

depending on what the intention is (the latter would restore the previous behaviour).

We could also allow predefining some wxDISABLE_ARRAYSTRING_INITLIST to exclude this ctor during the build, would this help?

@onitake
Copy link
Author

onitake commented Nov 27, 2023

Thanks for the explanation!

I worked around the issue with an explicit std::vector(), I think this is good enough for now.
As wxArrayString seems to be deprecated, it's probably better to refactor its usage anyway.
Do you have a timeframe when it will be removed completely?

@vadz
Copy link
Contributor

vadz commented Nov 27, 2023

Do you have a timeframe when it will be removed completely?

It's not going to be removed in the observable future, there is just too much code using it and it still must be used in some places in wx API. The plan/hope is to get rid of these remaining places and it's, of course, recommended not to use it in any new code, but removing it would break way too many code bases, so it's not going to happen any time soon.

vadz added a commit to vadz/wxWidgets that referenced this issue Nov 30, 2023
Provide a way to disable the new ctors taking std::initializer_list
added in 3.1.4 as they can break compilation of the existing code.

Closes wxWidgets#24093.
vadz added a commit to vadz/wxWidgets that referenced this issue Nov 30, 2023
Provide a way to disable the new ctors taking std::initializer_list
added in 3.1.4 as they can break compilation of the existing code.

Closes wxWidgets#24093.
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

No branches or pull requests

2 participants