#15206 closed enhancement (fixed)

Extending wxUniChar to Support Additional Types

Reported by: ByteMonk Owned by: vadz
Priority: low Milestone:
Component: base Version: stable-latest
Keywords: wxUniChar Cc:
Blocked By: Blocking:
Patch: yes

Description

The wxUniChar class needs to be extended to support the following types:

long long int
unsigned long long

The need for the long long int type was discovered when I attempted to use a CPPUnit and Google Mock-based test harness that referenced wxString. The test harness would not compile and generated this error in a Google Test header file:

"conversion from const wxUniChar' to long long int' is ambiguous"

Adding long long int to the constructor and various operators made the error go away. Context for the issue can be found here:

http://stackoverflow.com/questions/16549462/using-wxstring-with-google-mock/

Based on conversations with one of the wxWidgets developers I also included support for the unsigned long long type as part of this patch.

Attachments (2)

UniCharTypeAdditions.diff download (4.6 KB) - added by ByteMonk 17 months ago.
Patch detailing changes to the wxUniChar class
UpdatedUniCharTypeAdditions.diff download (30.5 KB) - added by ByteMonk 17 months ago.
Updated patch after implementing developer suggestions and including unit tests

Download all attachments as: .zip

Change History (8)

Changed 17 months ago by ByteMonk

Patch detailing changes to the wxUniChar class

comment:1 Changed 17 months ago by vadz

  • Status changed from new to confirmed

Thanks, but could you please update the patch to use wxLongLong_t and wxULongLong_t? Some old compilers (VC < 2005?) only support __int64 and not long long.

Also, I wonder if this was tested under 64 bits as I'd expect long long overloads to conflict with long there. So I think we need #ifdef wxHAS_LONG_LONG_T_DIFFERENT_FROM_LONG checks around them.

Of course, this would make the code even uglier than it already is. So it would probably be better to define a macro wxIF_LONG_LONG_TYPE() similar to the existing wxIF_WCHAR_T_TYPE() (warning, untested pseudo code):

// Helper macro for only doing something if wxLongLong_t is a real
// type different from other integer types.
#ifdef wxHAS_LONG_LONG_T_DIFFERENT_FROM_LONG
    #define wxIF_LONG_LONG_TYPE(x) x
#else
    #define wxIF_LONG_LONG_TYPE(x)
#endif

And then put all the additions inside it.

Thinking about it more, ideal would probably be to have a macro wxDO_FOR_ALL_INT_TYPES(body) which could be defined like this:

#define wxDO_FOR_ALL_INT_TYPES(body) \
    body(short) \
    body(unsigned short) \
    body(int) \
    body(unsigned int) \
    body(long) \
    body(unsigned long) \
    wxIF_LONG_LONG_TYPE( body(wxLongLong_t) ) \
    wxIF_LONG_LONG_TYPE( body(wxULongLong_t) ) \
    wxIF_WCHAR_T_TYPE( body(wchar_t) )

And then we could use it just like this:

#define wxUNICHAR_DEFINE_CTOR(type) wxUniChar(type c) { m_value = c; }

wxDO_FOR_ALL_INT_TYPES(wxUNICHAR_DEFINE_CTOR)

comment:2 follow-up: Changed 17 months ago by ByteMonk

I implemented your suggestions above and it didn't break any additional tests when I ran the test_gui. I wasn't able to use the wxDO_FOR_ALL_INT_TYPES in the wxDEFINE_UNICHAR_OPERATOR and wxDEFINE_UNICHARREF_OPERATOR sections. For those I left them expanded and used the wxIF_LONG_LONG_TYPE macro and wxLongLong_t and wxULongLong_t types as appropriate.

Where are most of these functions tested? The only test harness reference I saw to wxUniChar was in tests/strings/unichar.cpp. It doesn't look like the tests cover everything but if I am mistaken let me know. If it needs more coverage I can write some unit tests over the weekend.

comment:3 in reply to: ↑ 2 Changed 17 months ago by vadz

Replying to ByteMonk:

I implemented your suggestions above and it didn't break any additional tests when I ran the test_gui.

Great! Notice that there is also test in the same directory which contains most tests for the non-GUI stuff, so please run it too, as test_gui runs the GUI tests only.

I wasn't able to use the wxDO_FOR_ALL_INT_TYPES in the wxDEFINE_UNICHAR_OPERATOR and wxDEFINE_UNICHARREF_OPERATOR sections.

I see... We probably should add wxDO_FOR_ALL_INT_TYPES_1 (as we already do with wxFOR_ALL_COMPARISONS which has _1, _2 and _3 versions), i.e. (again, completely untested):

// In wx/defs.h
#define wxDO_FOR_ALL_INT_TYPES_1(body, arg) \
    body(short, arg) \
    body(unsigned short, arg) \
    body(int, arg) \
    body(unsigned int, arg) \
    body(long, arg) \
    body(unsigned long, arg) \
    wxIF_LONG_LONG_TYPE( body(wxLongLong_t, arg) ) \
    wxIF_LONG_LONG_TYPE( body(wxULongLong_t, arg) ) \
    wxIF_WCHAR_T_TYPE( body(wchar_t, arg) )

...

// In wx/unichar.h
#define wxDEFINE_UNICHAR_OPERATOR(op)                                         \
    bool operator op(const wxUniChar& c) const { return m_value op c.m_value; }\
    bool operator op(char c) const { return m_value op From8bit(c); }         \
    bool operator op(unsigned char c) const { return m_value op From8bit((char)c); } \
    wxDO_FOR_ALL_INT_TYPES_1(wxDEFINE_UNICHAR_CMP_WITH_INT, op)

#define wxDEFINE_UNICHAR_CMP_WITH_INT(T, op) \
        bool operator op(T c) const { return m_value op (value_type)c; }

Unfortunately I don't see how to define avoid redundancy between wxDO_FOR_ALL_INT_TYPES and wxDO_FOR_ALL_INT_TYPES_1 and while I have a feeling that it should be possible, I think duplicating the int types list in these 2 macros is not too bad as they're going to be defined near each other and so would be simple to update if we ever need to do it. Although if you do see any way to avoid duplication, it would be even better, of course.

Where are most of these functions tested? The only test harness reference I saw to wxUniChar was in tests/strings/unichar.cpp. It doesn't look like the tests cover everything but if I am mistaken let me know. If it needs more coverage I can write some unit tests over the weekend.

I think the tests are relatively complete but almost certainly not exhaustive so adding more would be always welcome. But if the existing tests pass, it's already a good sign.

Thanks again!

Changed 17 months ago by ByteMonk

Updated patch after implementing developer suggestions and including unit tests

comment:4 Changed 17 months ago by ByteMonk

I have attached a second patch file containing your suggestions above. I also added more unit tests to the test module for wxUniChar to ensure code coverage for the things that I added. All of the non-GUI tests pass.

I am getting failures for the GUI tests but they appear to be from timing issues with the wxUIActionSimulator. It looks like values entered in text fields are not ready at the time they are queried.

If you see any other issues with the patch let me know and I will take a look at them. Thanks for your help.

comment:5 Changed 17 months ago by vadz

  • Owner set to vadz
  • Status changed from confirmed to accepted

Thanks for your enhancements! Everything looks good to me, I'll apply this soon with some minor changes.

comment:6 Changed 17 months ago by VZ

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [74029]) Add conversions to/from long long to wxUniChar.

Allow conversions to/from long long and unsigned long long values in wxUniChar
for consistency with the other integral types.

Also make the code shorter by using helper wxDO_FOR_INT_TYPES() and
wxDO_FOR_CHAR_INT_TYPES() macros to avoid duplicating the same code for all of
the integral types and having to handle wchar_t (and wxLongLong_t now)
specially because sometimes we may need to overload on it and sometimes not.

Finally, add more tests to check that all the wxUniChar methods compile and
work with all the different types.

Closes #15206.

Note: See TracTickets for help on using tickets.