#10932: wxAny
wxTrac
#10932: wxAny
-----------------------------+----------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Keywords: wxAny wxVariant | Blockedby:
Patch: 0 | Blocking:
-----------------------------+----------------------------------------------
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.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: infoneeded_new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Changes (by vadz):
* status: new => infoneeded_new
Comment:
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!
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:1>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Changes (by jmsalli):
* status: infoneeded_new => new
Comment:
- 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!
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:2>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(by vadz):
Replying to [comment:2 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...
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:3>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(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?
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:4>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: infoneeded_new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Changes (by vadz):
* status: new => infoneeded_new
Comment:
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!
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:5>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Changes (by jmsalli):
* status: infoneeded_new => new
Comment:
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.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:6>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(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.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:7>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(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!
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:8>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(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.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:9>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(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.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:10>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(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!
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:11>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(by vadz):
Replying to [comment:11 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.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:12>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(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.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:13>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(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.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:14>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(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:
a. It's probably worth noticing that the default ctor creates a null (in
the sense that IsNull() returns true) object.
a. 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.
a. 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!).
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:15>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(by jmsalli):
Replying to [comment:15 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.
> 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.
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.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:16>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(by vadz):
Replying to [comment:16 jmsalli]:
> Replying to [comment:15 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.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:17>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(by jmsalli):
Replying to [comment:17 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.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:18>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(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?
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:19>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(by jmsalli):
Replying to [comment:19 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)").
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:20>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(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.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:21>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(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.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:22>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(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.
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:23>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: new
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(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!
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:24>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: closed
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: fixed | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Changes (by JMS):
* status: new => closed
* resolution: => fixed
Comment:
(In [61971]) wxAny initial commit (closes #10932)
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:25>
wxTrac
#10932: wxAny
--------------------------+-------------------------------------------------
Reporter: jmsalli | Owner:
Type: enhancement | Status: closed
Priority: normal | Milestone: future
Component: base | Version: 2.9-svn
Resolution: fixed | Keywords: wxAny wxVariant
Blockedby: | Patch: 0
Blocking: |
--------------------------+-------------------------------------------------
Comment(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 ;)
--
Ticket URL: <http://trac.wxwidgets.org/ticket/10932#comment:26>