wxWidgets / wxWidgets

Cross-Platform C++ GUI Library
https://www.wxwidgets.org/
5.74k stars 1.69k forks source link

wxString::c_str() does not allow implicit cast to (const wchar_t*) #9507

Closed wxtrac closed 15 years ago

wxtrac commented 15 years ago

Issue migrated from trac ticket # 9507

component: base | priority: normal | resolution: fixed | keywords: wxString c_str() implicit conversion cast

2008-05-29 09:30:10: @hajokirchhoff created the issue


The wxCStrData mechanism is flawed: wxMSW, Visual Studio 2005, Unicode, wxWidgets svn-trunk, wxUSE_STL

Hi, under the following circumstances code using .c_str() does not compile but fails with

-'Reason: cannot convert from 'wxCStrData' to 'const std::wstring' No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called'* Here is an example.

// This method accepts a const wchar_t*, which is implicitly converted to a std::wstring temporary by the compiler

-*void testme(const std::wstring &stlstring);** // These lines compile okay: '''testme(L"wide-string"); testme(wxString(L"wide-string"));''' // when wxUSE_STL

// this line does not: fails with error above testme(wxString(L"wide-string").c_str());

I expect .c_str() to return a const pointer of wchar_t to the contents of the string. This pointer would get converted to std::wstring automatically, implicitly.

// an explicit cast is required testme((const wchar_t*)wxString(L"wide-string").c_str());

but this defeats the purpose.

To sum it up: The .c_str() returning wxCStrData mechanism works only, if the callee (target function) expects a const wchar_t*. If the callee expects a datatype that would require an implicit conversion, returning wxCStrData from .c_str() does not work.

BTW, wxString::wc_str() does work in all cases.

I consider this a high priority as it breaks my existing code in many, many places. This used to work with wx 2.8 and earlier.

wxtrac commented 15 years ago

2008-05-29 11:01:30: @vslavik changed status from new to accepted

2008-05-29 11:01:30: @vslavik commented

Should be easily solvable for std::[w]string, but I'm afraid there's nothing we can do with it in general :-(

wxtrac commented 15 years ago

2008-05-29 11:15:38: @hajokirchhoff commented


Sorry, I did not follow the discussions regarding c_str(). Can you give me a quick pointer to the info why you are not simply returning a const wxChar * in that case?

That is what std::wstring does

const value_type *basic_string::c_str( ) const;

and I'd prefer wxString to be as much as std::wstring as possible. Especially since it should inherit from std::wstring when using wxUSE_STL, or am I wrong?

Regards

Hajo

wxtrac commented 15 years ago

2008-05-29 11:21:52: @hajokirchhoff commented


Replying to [comment:1 vaclavslavik]:

Should be easily solvable for std::[w]string, but I'm afraid there's nothing we can do with it in general :-(

And here is a related problem:

ofstream my_file; wxString a_file_name(L"test.dat"); my_file.open(a_file_name.c_str());

fails to compile with an 'ambiguous call to overloaded function' error.

Could be basic_ofstream<_Elem,_Traits>::open(const char *,std::ios_base::openmode,int)

or basic_ofstream<_Elem,_Traits>::open(const wchar_t *,std::ios_base::openmode,int)'

IOW, open accepts both, a const char and a const wchar_t and wxCStrData has two implicit conversions, one to const char and the other to const wchar_t.

My opinion here is: c_str() should return const char in ANSI (no longer available) and const wchar_t in UNICODE build.

This might break existing code when someone switches from ANSI to UNICODE, but that is a major change in a project anyway.

I don't see the advantage of a const char operator for wxString::c_str() in a Unicode Build. It should return const wchar_t always in that case.

Regards Hajo

wxtrac commented 15 years ago

2008-05-29 11:33:12: @vslavik commented


Replying to [comment:2 hajokirchhoff]:

Sorry, I did not follow the discussions regarding c_str(). Can you give me a quick pointer to the info why you are not simply returning a const wxChar * in that case?

http://docs.wxwidgets.org/trunk/overview_unicode.html http://wiki.wxwidgets.org/Development:_UTF-8_Support

This might break existing code when someone switches from ANSI to UNICODE, but that is a major change in a project anyway.

The whole point of all these changes was it would no longer be major change.

wxtrac commented 15 years ago

2008-05-29 11:59:07: @hajokirchhoff commented


Replying to [comment:4 vaclavslavik]:

http://docs.wxwidgets.org/trunk/overview_unicode.html http://wiki.wxwidgets.org/Development:_UTF-8_Support

Thank you.

The whole point of all these changes was it would no longer be major change.

Okay, this is your design decision and it's too late nor desirable to argue this point all over again. I might not have choosen this path. While it will work for people using wxWidgets as their only library, it will cause a major headache for others who use more than one library (i.e. STL, boost etc...) as it breaks an important interface between different kinds of strings.

Hm, perhaps it won't be so bad with an implicit conversion to std::wstring and std::string, enabled when wxUSE_STL is set to 1. std::string/wstring seems to be the only other string class around. Definitely worth a try.

Regards and thanks for your time and effort in wxWidgets

wxtrac commented 15 years ago

2008-05-29 12:11:00: @hajokirchhoff commented


And here is a related problem:

ofstream my_file; wxString a_file_name(L"test.dat"); my_file.open(a_file_name.c_str());

fails to compile with an 'ambiguous call to overloaded function' error.

How are we going to resolve this problem then? I was about to create a patch adding implicit conversion to std::[w]string, as you suggested, but this error is caused, because wxCStrData has two implicit conversions to char and wchar_t. This is very unexpected for other libraries, especially ones handling char and wchar_t in the same build (as opposed to MFC and wxWidgets that have a unicode build).

If I add both implicit conversions std::string and std::wstring, it will solve the first problem (the one I created the ticket for), but not this one (ambiguous call).

Worse, this will probably affect almost every function accepting strings in templated libraries such as boost. This ambiguity breaks automatic deduction of template arguments in functions like this:

template void process_a_string(const var_type *character);

The usual way of using this template method is simply calling it

process_a_string(my_string.c_str());

This works fine with std::[w]string my_string, but the wxString::c_str() ambiguity const char/const wchar_t will break template deduction for wxString.

Could we perhaps introduce a #define to enable/disable this feature? I consider it something like wxWidgets 2.6 backwards compatibility, which also can be disabled.

Regards

Hajo

wxtrac commented 15 years ago

2008-05-29 12:48:49: @vslavik commented


Replying to [comment:6 hajokirchhoff]:

ofstream my_file; wxString a_file_name(L"test.dat"); my_file.open(a_file_name.c_str());

fails to compile with an 'ambiguous call to overloaded function' error.

I must be missing something, but I don't see how that's possible, given that ofstream::open() has only const char* version in the standard.

How are we going to resolve this problem then?

Use mb_str() or wc_str(), as appropriate. In the rare case that the 3rd party library offers both overloads, use wc_str(). Once again, there's simply nothing that can be done about this (as far as I know -- if you know how to fix this, by all means, tell me).

This problem is going to be much less common than ANSI->Unicode breakage (as you said, it used to be major change), so it's arguably less important -- especially when it's trivially fixed (s/c_str/wc_str/g in relevant code in a way compatible with both wx3 and wx2.8).

Could we perhaps introduce a #define to enable/disable this feature?

I'd rather not, it's exactly the kind of setting that would cause endless trouble, because some ambiguities would exist only in some builds (unlike much simpler WXWIN_COMPATIBILITY_* differences).

wxtrac commented 15 years ago

2008-05-29 15:46:26: @vadz commented


I must be missing something but I don't understand what the problem is here: to make your code work it's enough to remove the call to c_str(). I agree that we can add conversion to std::[w]string to wxCStrData in the (default) wxUSE_STD_STRING==1 case but even without them this doesn't seem like a big deal.

wxtrac commented 15 years ago

2008-05-29 15:58:21: @vslavik commented


Replying to [comment:8 vadz]:

I must be missing something but I don't understand what the problem is here: to make your code work it's enough to remove the call to c_str().

Yes (for std::string; it wouldn't help at all with the template example above), but there's existing code that uses c_str() and it uses it because 2.8 didn't have implicit wxString->std::string conversion.

wxtrac commented 15 years ago

2008-05-30 11:01:22: @hajokirchhoff commented


Replying to [comment:8 vadz]:

I must be missing something but I don't understand what the problem is here: to make your code work it's enough to remove the call to c_str().

No, unfortunately not.

wxString openfile(L"afile");
std::ofstream file;
file.open(openfile);

will not work. ofstream::open expects a const char or const wchar_t and does not accept a std::[w]string. The example will fail.

wxString openfile(L"afile");
std::ofstream file;
file.open(openfile.c_str());

This will fail also, as explained in my ticket.

I will simply consider wxString::c_str() broken. It breaks the promise that it would be similar to std::[w]string::c_str(). This is a pity of course, since wxString is supposed to be used interchangeably and even inherits from std::wstring (does it not?)

This is going to be a problem for all programmers using wxWidgets and STL or boost together. OTOH people using wxWidgets exclusively probably wont notice it.

I completely understand that making the transition to Unicode as simple as possible for existing wxWidgets projects is a top priority for you. While the new c_str() semantic is annoying for me and goes against my programming instincts, I can live with it. I will simply search and replace all occurrences of c_str with wc_str.

If you give me a pointer, I will write the appropriate paragraph for 'breaking changes for wxWidgets 3.0' and then we can set this ticket to 'wontfix'. I wouldn't even bother adding implicit conversions to std::[w]string, as it would solve almost nothing.

Regards

Hajo

wxtrac commented 15 years ago

2008-05-30 11:21:09: @vslavik commented


Replying to [comment:10 hajokirchhoff]:

wxString openfile(L"afile"); std::ofstream file; file.open(openfile.c_str()); This will fail also, as explained in my ticket.

And as I already said, there's no ambiguity here -- read the standard. So if VC++ fails to compile this code, it's a bug in its standard library or the compiler itself. Can you please show us the exact error it gives (including references to what it thinks is ambiguous)?

wxtrac commented 15 years ago

2008-05-30 11:28:54: @hajokirchhoff commented


No ambiguity here? Sorry, what has the standard say to that?

Here is the exact error:

1>c:\users\hajo kirchhoff\desktop\prj\fis\project\mikado\data.cpp(11) : error C2668: 'std::basic_ofstream<_Elem,_Traits>::open' : ambiguous call to overloaded function 1> with 1> [ 1> _Elem=char, 1> _Traits=std::char_traits 1> ] 1> c:\program files\microsoft visual studio 8\vc\include\fstream(781): could be 'void std::basic_ofstream<_Elem,_Traits>::open(const char ,std::ios_base::openmode,int)' 1> with 1> [ 1> _Elem=char, 1> _Traits=std::char_traits 1> ] 1> c:\program files\microsoft visual studio 8\vc\include\fstream(753): or 'void std::basic_ofstream<_Elem,_Traits>::open(const wchar_t ,std::ios_base::openmode,int)' 1> with 1> [ 1> _Elem=char, 1> _Traits=std::char_traits 1> ] 1> while trying to match the argument list '(wxCStrData)'

std::ofstream::open has two overloads. One accepting const char, the other const wchar_t.

wxString::c_str() has two implicit conversions, one to const char*, the other to const wchar_t. That is causing the ambiguity.

How is that a bug in the Visual Studio 2005 compiler or STL? I don't see it, can you please explain?

Regards

Hajo

wxtrac commented 15 years ago

2008-05-30 11:34:55: @vslavik commented


Replying to [comment:8 vadz]:

I agree that we can add conversion to std::[w]string to wxCStrData

I was wrong about this -- we cannot add it to wxCStrData for the same reason wxString cannot have both implicit conversion to const char* and std::string: writing std::string(wxstr.c_str()) would be ambiguous.

wxtrac commented 15 years ago

2008-05-30 12:04:49: @vslavik commented


Replying to [comment:12 hajokirchhoff]:

No ambiguity here? Sorry, what has the standard say to that?

As I already wrote [comment:7 above], the standard says there's only one overload of ofstream::open(), the one that takes const char* (see 27.8.1.8). It's VC++ who is in error here. Of course, that's good to know, but not really helpful.

wxtrac commented 15 years ago

2008-05-30 13:27:10: @hajokirchhoff commented


Patch for 'changes.txt' explaining the situation and solutions.

If the patch is accepted, we can close the ticket.

wxtrac commented 15 years ago

2008-05-30 13:28:31: @hajokirchhoff uploaded file changes.txt.patch (1.4 KiB)

wxtrac commented 15 years ago

2008-05-30 15:15:45: @vadz commented


Looks like VC8 decided to solve the same problem wx3 solves in its own, standard-incompatible way. So while VC7.1 compiles this code just fine, VC8 and VC9 indeed fail. I really hate Microsoft idea that their STL is free to deviate from the standard in any way they like :-( And they don't provide any way to disable, just a comment saying that this is an extension:

        // open a wide-named C stream (old style) -- EXTENSION

which is great to know but completely unhelpful.

So this is indeed a big problem because I suspect they're going to do this with all the other functions in the future too but I don't see what can we do about this.

wxtrac commented 15 years ago

2008-05-30 15:27:51: @vadz changed priority from high to normal

2008-05-30 15:27:51: @vadz changed status from accepted to closed

2008-05-30 15:27:51: @vadz changed resolution from * to fixed*

2008-05-30 15:27:51: @vadz commented

I've added mention of c_str() ambiguities to docs/changes.txt and also docs/doxygen/overviews/changes_since28.h.

Your example with boost::regex compiles here just fine so I don't see what the problem is, as usual please provide more details if you reopen the bug, thanks.