Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#10932 closed enhancement (fixed)

wxAny

Reported by: jmsalli Owned by:
Priority: normal Milestone: future
Component: base Version: stable-latest
Keywords: wxAny wxVariant Cc:
Blocked By: Blocking:
Patch: no

Description

As discussed on wx-dev, wxAny is a proposed backwards-incompatible replacement for wxVariant. It is based on ideas of various variant implementations and relies heavily on C++ templates.

Attached is some working prototype code.

Some remarks about it:

  • wxAny itself holds type information (wxAnyValueType* m_type) and data buffer (wxAnyValueBuffer m_buffer).
  • A generic wxAnyValueTypeImpl template, which inherits from wxAnyValueType, should work for about 90% of data types (for others, a specialization of this template would need to be implemented). wxAnyValueTypeImpl uses wxIsMovable to see if type value is appropriate for simple storage in wxAny.m_buffer. If type is not "movable" or its size exceeds the buffer, then a holder object is allocated in heap (similar to existing wxVariant and boost::any behavior).
  • Brunt of the wxAnyValueTypeImpl's implementation is embedded in an intermediate wxAnyValueTypeImplBase class. This is done so that this intermediate class can be sub-classed, and then those subclasses can be used as basis for specialized wxAnyValueTypeImpl. For instance, in the first draft wxAnyValueTypeImplBase is used to create wxAnyValueTypeImplInt, which is then used as basis for specialized templates of all signed integer types. This allows for much smoother type conversion between those types.
  • For refined custom type conversion, you must create a specialized template and override virtual Convert() function. This is already done, for instance, in wxAnyValueTypeImplInt, to convert int to a wxString.
  • wxAnyValueType instances are singletons, allocated in global scope. wxModule is used to free these instances.

Attachments (8)

wxany_v1.cpp download (20.9 KB) - added by jmsalli 8 years ago.
wxany_v2.cpp download (24.5 KB) - added by jmsalli 8 years ago.
wxany_v3.cpp download (31.6 KB) - added by jmsalli 8 years ago.
wxany_v4.patch download (67.8 KB) - added by jmsalli 8 years ago.
wxany_v5.patch download (67.0 KB) - added by jmsalli 8 years ago.
wxany_final.patch download (101.4 KB) - added by jmsalli 8 years ago.
wxany_final_v2.patch download (67.8 KB) - added by jmsalli 8 years ago.
wxany_final_v3.patch download (63.5 KB) - added by jmsalli 8 years ago.

Download all attachments as: .zip

Change History (34)

Changed 8 years ago by jmsalli

comment:1 Changed 8 years ago by vadz

  • Status changed from new to infoneeded_new

Thanks for the patch!

Some comments:

  • Why do we need wxAnyNullValue? It would seem to be natural to just set m_type to NULL in the default constructor and when the object is made empty.
  • Speaking of which, I think we ought to have Clear() method too. Currently you need to assign wxAny() to an existing object to clear AFAICS.
  • My main problem with current version is the use of implicit conversion operator. I strongly believe this is too dangerous. The implicit ctor/assignment operator is probably too convenient to not have it but extraction should be explicit via some bool GetAs(T *value) method. And we could possibly also have const T& As() but it should throw (well, call some wxThrow() function which wouldn't throw if wxUSE_EXCEPTIONS==0 but still do something very disruptive) if the type is not correct.
  • On a similar note, I don't think operator==() should be using conversion. IMO there might be a different function to do what it does now but this operator should just return false if the types don't match.
  • I also don't understand at all what is operator wxAny() const good for?
  • Minor remark: wxUSE_WCHAR_T is always 1 now.
  • wxAnyValueType:
    • It looks IsSameType() doesn't need to be virtual as it is always implemented by comparing the results of calling GetClassInfo() on this and the other object.
    • The name DynamicSetData() is mysterious. And I couldn't understand what was it for as it doesn't seem to be used anywhere.
    • I think all methods should be virtual, I don't see why e.g. EqOfSameType() should have its dummy implementation. Alternatively (see below), get rid of this class at all and make all of its methods (except ConvertValue()) non-virtual.
  • Another important question: testing for wxIsMovable<T>::value in an if statement is really not how it's supposed to work. Instead you should define 2 specializations of wxAnyValueTypeImplBase, one for movable types and another for the rest. Like this you gain in efficiency both by getting rid of the if statements and by avoiding the need for virtual function call. See wx/vector.h for how it's done for wxVectorMemOpsMovable/Generic there.
  • I think WX_ANY_BASE_SIGNED_INT_TYPE is unnecessary and is just cryptic/confusing, anybody can use long when they need it. Besides it should probably be wxLongLong and not long anyhow.
  • Last important comment:I don't understand why is wxAnyValueTypeGlobalsManager and everything related to it needed at all.
  • It would be better to use static_cast<> more instead of C style casts which make the code rather difficult to understand at times and might also lose us some safety which could be gained from using static cast.
  • And a style comment because no patch review is complete without one :-): please use the standard m_ prefix for member variables, even if it's in a union (or, alternatively, use no prefix at all). v_ prefix really made me wonder where did it come from as AFAIK it's not used anywhere else in wx. Also, all the private stuff should be in wxPrivate namespace to avoid polluting the global one.

Thanks again for working on this!

comment:2 follow-up: Changed 8 years ago by jmsalli

  • Status changed from infoneeded_new to new
  • By having wxAnyNullValue I have tried to make sure that m_type is never NULL and so there's less checks to be made in code. It is an optimization, mostly.
  • Yes, Clear() or MakeNull() will be needed. Current code is of course missing a lot of basic utility functionality.
  • I agree that implicit conversion from wxAny is dangerous (although convenient - but dangerous nonetheless). I will add GetAs(T*), it makes sense. Maybe Cast() would be better name for As()? AFAIK C++ cannot differentiate template functions based on return value alone, so it would have to be called Cast<T>() or As<T>(), even from the user code.
  • You are probably right with == dynamic conversion. Removing it will make things more consistent.
  • About "operator wxAny() const": looks like this is indeed superfluous.

wxAnyValueType:

  • I think IsSameType() needs to be virtual. It calls static member function IsSameClass() that is only defined in derived classes.
  • Sorry, DynamicSetData() was just some unused code I forgot to remove. Also, StaticSetData() should/will be renamed to SetValue().
  • You mean all virtual functions should be abstract? I guess you are right, will change this.
  • wxIsMovable<T> use: Ok, I'll try the same trick. Looks clever. Anyhow, I had hoped that compilers would've been smart enough to eliminate those conditions anyway, static as they are. However, at this point I'm not certain whether "sizeof(T) <= WX_ANY_VALUE_BUFFER_SIZE" tests can be eliminated in the same way. I'm hoping it can be embedded in the same condition in wxIf.
  • I used cryptic WX_ANY_BASE_SIGNED_INT_TYPE because I wasn't yet sure whether long or wxLongLong was preferred. I was also not sure if this model would work with wxLongLong, as it is not necessarily a native integer type. Naturally things would be ok with wxLongLong_t. I will look more into this.
  • Ok, I'll use static_cast<> more. To begin with, I was going to use wxStaticCast (I thought that was the right thing instead of static_cast<>) but it seems to require wxObject inheritance (or then there was some other problem with it, sorry can't remember exact details).
  • wxAnyValueTypeGlobalsManager: It is not needed if you think wxAnyValueTypeGlobals itself can derive from wxModule.
  • Ok, will fix the prefixes and use wxPrivate (which I was not aware of before).

Hopefully that addressed all of your questions.

Thanks!

comment:3 in reply to: ↑ 2 Changed 8 years ago by vadz

Replying to jmsalli:

  • By having wxAnyNullValue I have tried to make sure that m_type is never NULL and so there's less checks to be made in code. It is an optimization, mostly.

I didn't realize it was an optimization, sorry. It's probably worth mentioning it in a comment.

  • I agree that implicit conversion from wxAny is dangerous (although convenient - but dangerous nonetheless). I will add GetAs(T*), it makes sense. Maybe Cast() would be better name for As()? AFAIK C++ cannot differentiate template functions based on return value alone, so it would have to be called Cast<T>() or As<T>(), even from the user code.

I like the similarity between GetAs() and As(). With Cast() you'd probably need to call them both like this. There is also the fact that As() is shorter which is important for a method which will be used most often with this class probably.

Finally, Cast() would be the right choice if we strived for some compatibility with boost::any but IMO we don't.

wxAnyValueType:

  • I think IsSameType() needs to be virtual. It calls static member function IsSameClass() that is only defined in derived classes.

Yes, you're right, I missed this one.

  • You mean all virtual functions should be abstract? I guess you are right, will change this.

Yes and thanks.

  • wxIsMovable<T> use: Ok, I'll try the same trick. Looks clever. Anyhow, I had hoped that compilers would've been smart enough to eliminate those conditions anyway, static as they are. However, at this point I'm not certain whether "sizeof(T) <= WX_ANY_VALUE_BUFFER_SIZE" tests can be eliminated in the same way. I'm hoping it can be embedded in the same condition in wxIf.

Yes, this should work the same way. And this "clever" trick is standard idiom in "template meta-programming" so I think using it can actually be more clear too.

  • Ok, I'll use static_cast<> more. To begin with, I was going to use wxStaticCast (I thought that was the right thing instead of static_cast<>)

Not any more in wx 2.9, we require a compiler supporting at least this.

  • wxAnyValueTypeGlobalsManager: It is not needed if you think wxAnyValueTypeGlobals itself can derive from wxModule.

What I mostly meant was that I didn't understand why did we need these dynamic allocations at all. Maybe we should discuss this more on wx-dev?

  • Ok, will fix the prefixes and use wxPrivate (which I was not aware of before).

It's indeed not documented. Unfortunately I don't know where could we document it neither...

comment:4 Changed 8 years ago by jmsalli

Here's a new revision.

Changes:

  • Using wxVector style wxIf<...> Ops now in wxAnyValueTypeImplBase. I'm new to "template meta-programming", so for me most things done with it seem very clever indeed ;)
  • Removed implicit conversions, added GetAs(T*) and As<T>() (BTW, which both currently use dynamic conversions).
  • Added unsigned ints as well, as a separate type that is basically incompatible with signed int type (eg. wxAny((int)1) == wxAny((unsigned int)1) is false).
  • If wxLongLong_t available, it is used as base int type. Same goes for wxULongLong_t. Changed WX_ANY_BASE_SIGNED_INT_TYPE into something less verbose.
  • Dynamic conversions between above types and strings.
  • wxAnyNullValueType implementation moved to the source portion.
  • As discussed: using static_cast<> and reinterpret_cast<> instead of C-style casts, using wxPrivate as much as possible (I think), all base wxAnyValueType virtual functions are now abstract, operator==() no longer does dynamic conversion, union prefixes fixed.

Other Remarks:

  • About wxAnyValueTypeGlobals: It is still there... I was not sure if it was safe to have non-POD, non-pointer static template member variables. If you think it is safe, I will of course remove the globals class (as it indeed serves no other purpose except to release wxAnyValuType instances allocated in heap).
  • About wxPrivate: Maybe it could be mentioned in the wxWidgets Coding Guidelines?

Changed 8 years ago by jmsalli

comment:5 Changed 8 years ago by vadz

  • Status changed from new to infoneeded_new

Global comments:

  • This starts looking really good! I guess only docs and real unit tests are needed now, thanks for writing this so nicely!
  • Probably the main (only?) problem I have is that it starts getting quite complex by now, it would be nice to have a comment explaining how all the various classes interact with/relate to each other.
  • I guess that use of some macros (WX_ANY_DEFINE_INT_TYPE) is inevitable to avoid needless repetition but I have my doubts about the others, e.g. WX_DECLARE_ANY_VALUE_TYPE is only used once and WX_DECLARE_ANY_VALUE_TYPE_IN_CLASS twice, maybe we could get rid of it? And WX_ANY_VALUE_BUFFER_SIZE and wxAnyTypeCheck should definitely be a const/enum and inline template function respectively and not macros.
  • Finally I'm still bothered by wxAnyValueTypeGlobals, I don't understand what is it for so maybe it's better if you removed it and then we reintroduced it back if/when we run into problems?

Random small comments from reading the code:

  • wxAnyValueTypeOpsMovable::EqOfSameType() can be written simply as GetValue(buf1) == GetValue(buf2), no need to copy to temporary objects.
  • wxAnyValueTypeOpsGeneric
    • Should DataHolder objects be copyable? I don't think so (and then wxDECLARE_NO_COPY_CLASS should be used in it).
    • CopyBuffer() could reuse DeleteData() instead of duplicating it -- not a big deal but it's IMO cleaner to always allocate and delete things in only one place.
    • As above, EqOfSameType() could reuse GetValue()
  • wxAny
    • I still prefer Clear() to MakeNull() but well, hardly crucial
    • Why duplicate the same code in As() and GetAs() instead of implementing the former in terms of the latter?

Thanks again!

comment:6 Changed 8 years ago by jmsalli

  • Status changed from infoneeded_new to new

Thanks for the review. Looks like there was indeed a lot of code redundancy left in. A new revision attached.

Remarks:

  • I've added some comments/documentation with some information about class relations. From my perspective it is a bit difficult to see which parts require clarification.
  • I think WX_DECLARE_ANY_VALUE_TYPE() is useful shortcut if user needs to declare really custom wxAnyValueType, ie. one that is not derived from wxAnyValueTypeImplBase. For instance, if we were to add optimized wxString value type (one that would copy short strings into the buffer), this macro could be used. Although I'm not sure if this particular example could not also be implemented with some template technique. Maybe by defining specialized wxPrivate::wxAnyValueTypeOpsMovable<> for wxString?
  • IMHO Clear() is more confusing as it is also used in the context of lists, hash maps, and other similar containers. Also, MakeNull() is consistent with existing wxVariant API.
  • I tried the code without wxAnyValueTypeGlobals, and things went wrong. I realize now that this is because wxAnyValueType is a polymorphic class and such instances must be allocated with "new".

To explain this better, look at this global code that implements the static member variable:

template<typename T>
wxAnyValueTypeImplBase<T>* wxAnyValueTypeImplBase<T>::sm_instance =
    new wxAnyValueTypeImpl<T>();

If the member variable would not be a pointer, then the code would look like this:

template<typename T>
wxAnyValueTypeImplBase<T> wxAnyValueTypeImplBase<T>::sm_instance;

Result is that the sm_instance would become instance of wxAnyValueTypeImplBase<T> instead of wxAnyValueTypeImpl<T> as it was supposed.

Changes and improvements:

  • Dynamic conversions for bool and double. Do we need this for any other types?
  • Float and double use same base wxAnyValueType, in similar manner as all signed integer types use one.
  • Based on your observations and suggestions: wxAnyValueCheck is template function, DataHolder uses wxDECLARE_NO_COPY_CLASS, As() calls GetAs(), eliminated Ops::EqOfSameType() and Ops::CopyBuffer().

I'll start to work on a "real" patch soon, with cpp/h files in place, documentation and of course some unit tests.

Changed 8 years ago by jmsalli

comment:7 Changed 8 years ago by jmsalli

Ok, here's another revision (by no means final version yet). This time in a real patch format.

Changes:

  • Because it wouldn't compile otherwise in VC7/8 shared build, I had to move wxAnyValueType's sm_instance variable upwards in (template) class hierarchy, so it is now always in wxAnyValueTypeImpl<>. A bit more macro madness for those who like to customize their wxAnyValueType.
  • I think wxAny::As<T>() is going to used 99% of the time so that no dynamic conversion is needed, so I've removed that conversion for the time being. Of course, this is basically just an optimization to reduce inline code bulk. Naturally GetAs(T*) still does the conversion.
  • Removed global wxAnyValueCheck<>(). Now instead there are wxAny::CheckType<T>() and wxAnyValueType::CheckType<T>().
  • Renamed DeleteData() to DeleteValue(), just so it'd be more in line with GetValue() and SetValue().
  • Added some preliminary inline doxygen documentation in any.h.
  • No unit tests included yet. Updated version of that test code is in a commented-out section at the bottom of any.h.

Changed 8 years ago by jmsalli

comment:8 Changed 8 years ago by vadz

Sorry, I still didn't have time to understand the problem with sm_instance so I can't say anything about it. Other than this, I glanced over the latest version and it looks good, I'll try to look at it in more details later but I could just as well look at the next version too so please don't let me delay your work.

Just one thing I'd wish to see in the test or documentation (or both): could we have a simple example showing how to define a custom wxAnyValueType<> specialization?

TIA!

Changed 8 years ago by jmsalli

comment:9 Changed 8 years ago by jmsalli

I guess there is no real problem with sm_instance, it just the sole reason for the existence of wxAnyValueTypeGlobals class. A change I mentioned earlier, regarding sm_instance placement in class hierarchy, did allow me to allocate instances without "new" keyword, and thus I got rid of the 'globals' class... until things broke down when I was testing on GCC 4.2.3. So, sm_instance remains a pointer for now, and 'globals' class is needed to keep track of allocated instances, and a separate wxModule is needed (AFAIK) to properly delete those instances.

The v5 patch should compile on *nix systems with GCC 4.2. Earlier versions definitely did not. I had to make class identification code a bit more bulkier than the one that worked on VC8. There is a vague note about it in the header file.

About wxAnyValueType specialization: There should already have been a small sample about that, in any.h comments/docs. I'll see if I can reasonably add one in the (unit) tests as well.

comment:10 Changed 8 years ago by jmsalli

Attached is now the first "release candidate" for final patch. Included are documentation and unit tests. No real functionality changes from previous revision.

As soon as you give me a green light, I'll commit files into the SVN trunk.

comment:11 follow-up: Changed 8 years ago by jmsalli

I'm having some problems with VC6. Looks like use of CheckType<T>() and As<T>() template functions (like in "double d = any.As<double>();") refuse to be compiled. Error appears as "error C2062: type 'double' unexpected".

I've worked around this in any.cpp by using a macro instead of CheckType<T>(), but compiling the unit tests naturally fails.

Any ideas?

Thanks!

comment:12 in reply to: ↑ 11 Changed 8 years ago by vadz

Replying to jmsalli:

I'm having some problems with VC6. Looks like use of CheckType<T>() and As<T>() template functions (like in "double d = any.As<double>();") refuse to be compiled.

Yes, VC6 doesn't support explicitly specifying the template function to call. You need to do something like this

// FIXME-VC6: remove this hack when VC6 is no longer supported
template <typename T>
bool CheckType(T * = NULL);

#define wxANY_CHECK_TYPE(any, t) any.CheckType(static_cast<t *>(NULL))

and use wxANY_CHECK_TYPE in wx code itself. For the unit tests you could also just put a check for VC6 around CheckType() calls.

comment:13 Changed 8 years ago by jmsalli

Updated wxany_final.patch with some VC6 compatibility. That is, library and unit tests should build with a healthy compiler (I think). Linking the unit tests even in static build gives me internal compiler errors, so I can't run it.

Some notes:

  • For some reason I got errors ("cannot specialize template function for type X") when doing what you suggested with CheckType(), so I just inlined that single-line function as a macro instead.
  • For some other reason, that same method seemed to work fine with As<T>() counterpart wxANY_AS(). That is, wxanytest.cpp compiled without errors.

Changed 8 years ago by jmsalli

comment:14 Changed 8 years ago by jmsalli

Added test case for user data type, fixed some warnings with GCC when running the tests. Tweaked the documentation just a bit.

Now I have a design question, however: Currently, user data type is required to have operator==() implementation for the default wxAnyValueTypeImpl<> template to compile. We could drop this requirement if the default wxAnyValueTypeImpl<>::EqOfSameType() implementation would be a dummy one. Then as a consequence, for wxAny::operator==(const wxAny&) to work for a data type, a specialized wxAnyValueTypeImpl<> template would have to be created for that type.

So, which would be preferred here, operator==() requirement for user data types or chance for some inconsistency in direct wxAny to wxAny comparison? Of course, as an alternative, implicit wxAny to wxAny comparison could be dropped altogether. For a reference, boost::any doesn't have any == operators at all (or at least that is how it looks like to me).

BTW, I hope you don't mind that I left bakefile-generated changes into the patch. It was getting a chore to remove them manually for every revision of the patch.

comment:15 follow-up: Changed 8 years ago by vadz

Sorry for the delay, I'm finally reading the patch again and so far only found trivial issues:

  1. tests/any/wxanytest.cpp would IMO be better named tests/any/anytest.cpp for consistency with the other test files. It also has a wrong file name in its "Name" comment in the header and AFAICS doesn't need to include wx/wx.h at all.
  2. Instead of using feq() you could simply use CPPUNIT_ASSERT_DOUBLES_EQUAL(). It does require specifying the delta for each comparison but IMO it's worth the trouble as using it is more clear.
  3. It'd also be nice to use CPPUNIT_ASSERT_EQUAL(a1, a2) instead of CPPUNIT_ASSERT(a1 == s2). This does require writing an operator<<(ostream&) for wxAny (which could be useful anyhow BTW) but it provides valuable information when the tests fail and is well worth it.
  4. Comment preceding wxAnyValueType seems to be incomplete.
  5. There is "excepted" which was probably supposed to be "expected" in CopyBuffer() comment.
  6. Documentation:
    1. It's probably worth noticing that the default ctor creates a null (in the sense that IsNull() returns true) object.
    2. IMO ctors from const char * and wchar_t don't need to be documented, this is just an implementation detail and might change in the future. Same for assignment operators.
    3. I'm not sure if you use
      wxAny anyUlong(static_cast<unsigned long>(128));
      

for extra clarity but I'd just write wxAny anyUlong(128UL); instead. Although maybe this example is moot anyhow because of the considerations below.

The only more serious (albeit old) problem I see is that the use of g_wxAnyValueTypeGlobals makes the code MT-unsafe which is a problem IMO. I still didn't have time to dive into the details and understand why is this needed but I just can't believe it's really necessary to allocate this stuff on the heap at all :-( I might try to make it work without it once you check the code in -- with the unit test it should be simple to detect whether it still works or not.

So, which would be preferred here, operator==() requirement for user data types or chance for some inconsistency in direct wxAny to wxAny comparison?

I think we could use another compile-time dispatch to use different specializations for classes with and without assignment operator. Whether it's worth the extra complexity is another question... Maybe not providing comparison operator at all is indeed better, at least initially -- we can always add it later if it turns out to be really needed. But I think that requiring people to write

    if ( any1.IsOfSameType(any2) && any1.As<T>() == any2.As<T>() )
       ... they're equal ...

in their code is not such a big deal neither. And it's definitely better than providing comparison operator which compiles but doesn't do the right thing, this is definitely a bad idea.

And FWIW I think that the fact that comparing 128UL with 128 currently fails is bad too. This is really unexpected and I can't see any situation in which this behaviour could be desirable. I do understand that fixing it might be difficult so it's another argument for not providing comparison operator at all and tell people to do

    int n1, n2;
    if ( any1.GetAs(&n1) && any2.GetAs(&n2) && n1 == n2 )

themselves.

BTW, I hope you don't mind that I left bakefile-generated changes into the patch.

It's not a big problem, of course, but OTOH you could also just do

svn diff -x -w in*/wx/any.h src/common/any.cpp tests/any build/bakefiles/files.bkl include/wx/setup_inc.h

instead of removing them manually (which would indeed be painful!).

comment:16 in reply to: ↑ 15 ; follow-up: Changed 8 years ago by jmsalli

Replying to vadz:

Sorry for the delay, I'm finally reading the patch again and so far only found trivial issues:

Should be mostly fixed now, thanks.

  1. It'd also be nice to use CPPUNIT_ASSERT_EQUAL(a1, a2) instead of CPPUNIT_ASSERT(a1 == s2). This does require writing an operator<<(ostream&) for wxAny (which could be useful anyhow BTW) but it provides valuable information when the tests fail and is well worth it.

As I have now "removed" direct wxAny to wxAny comparison (see below) this does not seem to be necessary, especially since CPPUNIT_ASSERT_EQUAL(a1, a2) only seems to work if both a1 and a2 are wxAny. Eg. CPPUNIT_ASSERT_EQUAL(any, 128) fails to compile for me.

The only more serious (albeit old) problem I see is that the use of g_wxAnyValueTypeGlobals makes the code MT-unsafe which is a problem IMO. I still didn't have time to dive into the details and understand why is this needed but I just can't believe it's really necessary to allocate this stuff on the heap at all :-( I might try to make it work without it once you check the code in -- with the unit test it should be simple to detect whether it still works or not.

I'm far from a MT expert, but I don't really understand why this sort of heap allocation would be problem in multi-threaded applications. All of it happens in global scope, before any thread has chance to start (AFAIU), and after that nothing is written to g_wxAnyValueTypeGlobals. Using wxModule for deallocation should be MT-safe too, no?

So, which would be preferred here, operator==() requirement for user data types or chance for some inconsistency in direct wxAny to wxAny comparison?

I think we could use another compile-time dispatch to use different specializations for classes with and without assignment operator.

Well, unfortunately I have no idea how to do it (transparently), so it'd be great if you could share your thoughts in more detail.

Anyway, I've now just replaced direct wxAny-to-wxAny comparison with dummy one that wxFAIL()s at run-time. I really had to add that operator explicitly, as otherwise it would compile with the default template-based operator==(). Do you know a way to have this dummy operator==(wxAny) to fail at compile-time instead of just run-time?

Also, removed wxAnyValueType::EqOfSameType() for the moment, since it is not called anywhere in the current code.

And FWIW I think that the fact that comparing 128UL with 128 currently fails is bad too.

I have now fixed this by adding explicit integer comparison operators, which take into account both signed and unsigned wxAnyValueType int classes.

Changed 8 years ago by jmsalli

comment:17 in reply to: ↑ 16 ; follow-up: Changed 8 years ago by vadz

Replying to jmsalli:

Replying to vadz:

The only more serious (albeit old) problem I see is that the use of g_wxAnyValueTypeGlobals makes the code MT-unsafe which is a problem IMO.

I'm far from a MT expert, but I don't really understand why this sort of heap allocation would be problem in multi-threaded applications. All of it happens in global scope, before any thread has chance to start (AFAIU), and after that nothing is written to g_wxAnyValueTypeGlobals. Using wxModule for deallocation should be MT-safe too, no?

Yes, you're right, sorry, I didn't realize this was done at global initialization time and not when a wxAny was used for the first time as I thought for some reason.

Anyway, I've now just replaced direct wxAny-to-wxAny comparison with dummy one that wxFAIL()s at run-time. I really had to add that operator explicitly, as otherwise it would compile with the default template-based operator==().

Err, there is no such thing as default operator==(), you're probably thinking about the assignment operator. If you don't define the comparison operator, using it will result in a compilation error so all you need to do is to omit it.

comment:18 in reply to: ↑ 17 Changed 8 years ago by jmsalli

Replying to vadz:

Err, there is no such thing as default operator==(), you're probably thinking about the assignment operator. If you don't define the comparison operator, using it will result in a compilation error so all you need to do is to omit it.

Sorry, I was probably not clear enough. With default template operator==() I meant the existing bool operator==(const T& value) const, which also happens to compile if T is wxAny, and will always return false. At least this is how it worked for me with VC8.

comment:19 follow-up: Changed 8 years ago by vadz

I'm afraid this is still not clear enough for me... where does this existing template operator come from and why don't we remove it?

comment:20 in reply to: ↑ 19 Changed 8 years ago by jmsalli

Replying to vadz:

I'm afraid this is still not clear enough for me... where does this existing template operator come from and why don't we remove it?

It is in wxAny, and I don't recall that we decided to completely scrap the operator==(). In fact, the current code is just a candidate with potential solution to wxAny-to-wxAny comparison problem, and the generic template-based comparison operator is "getting in the way", so to speak. Naturally, if it looks like things will be overall better without that particular operator at all, then I guess we must remove it. Of course, special cases for built-in types (ints, wxString, etc.) could be left in. Also, I guess we could add template function for generic comparison (eg. "bool Equals(const T& value)", though that would still allow user to add non-working code like "any1.Equals(any2)").

comment:21 Changed 8 years ago by vadz

Ok, that's the source of confusion -- I thought we did decide to remove the comparison operator entirely seeing that it's non obvious (if possible at all) to implement correctly. And I still don't see how could it possibly be implemented correctly for all types so I still think this would be the best thing to do.

Having wxAny::operator==(int) (and for wxString too maybe) might be convenient though, I hesitate about this one... But OTOH I also could live without this one too and extract an integer value before comparing it from it myself, especially as it would be still needed for other comparisons. Does this

   if ( a.As<int>() == 17 )

really look that much worse than

   if ( a == 17 )

? And it's more explicit and hence more clear. And you could also do

   int n;
   if ( a.GetAs(&n) ) {
      if ( n == 17 )
          ....
   }
   else {
      ....
   }

and handle what happens if the object is not an int at all explicitly if needed.

comment:22 Changed 8 years ago by jmsalli

Right, I'll remove the generic operator in the next revision of the patch. I think having specific wxAny::operator==() functions for ints, wxString, and double could still be useful, and int operators are there already so it's not even too much work either.

Also,

    if ( a.As<int>() == 17 )

is not exactly the same as

    if ( a == 17 )

since operator==() always checks the type, while As<>() wxFAIL()s and returns a dummy value if the type doesn't match. Of course, as your third snippet demonstrated, GetAs() can be used for type-safety, but the code involved is much more verbose.

Changed 8 years ago by jmsalli

comment:23 Changed 8 years ago by jmsalli

Ok, attached a new patch. Removed operator==(const T& value), added specific operator==()s for all integer types, float, double, wxString, char*, wchar_t*, and bool.

Revised and fixed various parts of the documentation. Added a bit about movable data being stored in more optimal manner, and how to make user data movable.

comment:24 follow-up: Changed 8 years ago by vadz

Jaakko, I'm awfully sorry but I completely forgot to look at the new version of the patch. I hope it's not too late to return to it.

I don't have any new comments, just more about comparisons: if we allow comparing any integral types with int, shouldn't we also allow comparing floats and doubles? Or does it work already because of "subtype" magic (which I didn't really understand)?

Also, operator!=() could probably be just a template to avoid typing. But this is not very important, especially as you already did type all this :-)

On a related topic, maybe a GetAsDouble() could be useful to retrieve the value of either float or double. But it can certainly be added later if the need for it really arises, there is no call to do everything at once.

Anyhow, I think it's time to commit this and fix remaining issues in svn.

TIA!

comment:25 Changed 8 years ago by JMS

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

(In [61971]) wxAny initial commit (closes #10932)

comment:26 in reply to: ↑ 24 Changed 8 years ago by jmsalli

I changed the operator!=() into a template, thanks for pointing it out.

Float/double comparison should already work due to the "subtype" magic. I'll see that there is such comparison in the tests, if not already.

GetAsDouble() would in effect be same as GetAs<double>(), again thanks to the subtype magic ;)

Note: See TracTickets for help on using tickets.