Yarp: Problem with Value::operator==() operating on Value& objects?

[pattacini]

Recently, I and @Tobias-Fischer came across the following unexpected (at least for us) behavior of Value::operator==() when dealing with lists and reference objects (i.e. Value&).

The following snippet:

Bottle b1;
Bottle &s1=b1.addList();
s1.addString("prop");
s1.addDouble(1.0);

Bottle b2=b1;

Value v1=b1.get(0);
Value v2=b2.get(0);
Value &w1=b1.get(0);
Value &w2=b2.get(0);

yDebug()<<(v1==v2?"ok":"fail");
yDebug()<<(w1==w2?"ok":"fail");
yDebug()<<(v1==w1?"ok":"fail");

produces the following output:

[DEBUG]ok
[DEBUG]fail
[DEBUG]ok

It seems that when we both deal with Value&, as Bottle::get() actually returns a reference, the operator==() does not make its job correctly. Is this happening on purpose or is it a bug?

[pattacini]

@drdanz do you have any insight on this?

[drdanz]

Sorry for the mess with the assignee, my phone started doing things by itself!
No, I think this is a bug... I'll have a look at that as soon as possible...

[Tobias-Fischer]

Hi @pattacini and @drdanz,
I've had a look at this and found a fix for it. However, the syntax is ugly so I want to check with you guys first whether there is a nicer way of doing it.

The issue is in the implementation of bool Value::operator==(const Value& alt) const. If one prints (*proxy)->toString(), one can see it's just an empty string. This empty string is then passed to bool Storable::operator==(const Value& alt) const, which performs a string comparison: return String(toString().c_str()) == alt.toString().c_str();. Obviously, this fails if one string is empty.

The fix I found is as follows;

diff --git a/src/libYARP_OS/src/Value.cpp b/src/libYARP_OS/src/Value.cpp
index 9ef5ba4..722c45a 100644
--- a/src/libYARP_OS/src/Value.cpp
+++ b/src/libYARP_OS/src/Value.cpp
@@ -288,7 +288,7 @@ Bottle& Value::findGroup(const ConstString& key) const
 bool Value::operator==(const Value& alt) const
 {
     ok();
-    return (*proxy)==alt;
+    return *(Storable*)this==alt;
 }

This is working fine with the snippet posted by @pattacini above (and also throws a non-match if the two yarp::os::Bottles are not the same. However, the syntax *(Storable*)this doesn't look very nice. Is there a better way?
If not, I'll open a pull request.

Best,
Tobi

[pattacini]

Cool :tada:
I'd rather change the _C-style_ cast in an explicit _C++_ cast by doing:

- return *(Storable*)this==alt;
+ return *static_cast<Storable*>(this)==alt;

[drdanz]

I wonder why it is comparing the _proxy_ with the alt Value&
IMHO it should be proxy == alt.proxy, but before jumping to conclusion I want to investigate this a little bit more, there might be some weird reasons to that (Probably because the Storable::operator==(const Storable& alt) is not implemented, only Storable::operator==(const Value& alt)).

Also the implementation of the operator== does not seem very reasonable in some cases (see the FIXMEs in the tests/libYARP_OS/ValueTest.cpp file). It uses toString() for everything, but in some cases (i.e. when both terms are numerical values) it should, in my opinion, do a numerical comparison. I wanted to change this some time ago, but I was afraid people might rely on "10" being not equal to "10.0".

It would be nice to add this case (and perhaps some other similar) to the test suite...

[pattacini]

As per the internal discussion with @drdanz, it's not that clear at the moment whether we should go for something different from _string comparison_, given that type-specific comparisons in nested lists might bring about performance issues.

On the other hand, we can make very evident in the methods' documentation that the comparisons will be done according to some criteria.

[Tobias-Fischer]

Hi,
@drdanz: I didn't get the comparison between the two proxies to work. That's why I went for the solution above.
@pattacini: Yes, the C++ static_cast is indeed much nicer.

Regarding the comparison itself: I guess that type-specific comparisons would be nicer. Maybe this could be introduced in the next major version of yarp, clearly indicating that it breaks compatibility. However, I guess most people are not affected by a (double)10.0==(int)10 to be true ..

For now, I guess we should fix the bug one way or the other (i.e. casting to Storable or comparing the proxies).

[pattacini]

@Tobias-Fischer my main concern was about comparing very long and nested Value objects, i.e. containing sub-lists. In this context, I don't know whether comparing a unique long string will run significantly faster than comparing against each single property of the sub-list.

We should check this carefully, since despite yarp::os::Value might appear mazy, it is used everywhere...

However, performance issues aside, changing the meaning of the comparison shouldn't affect any current code, I think.

[Tobias-Fischer]

Ahhh, I see what you mean. Good point indeed, this would need some more extensive tests I guess.

[drdanz]

@pattacini I'm not sure if it will be slower, probably it depends on the cases, since toString() already descends in every object of the sublist to produce the final string, probably doing string copies and memory allocations, and in the end, a comparison between strings is performed...
We could do some tests though.

[pattacini]

:+1: for accurate tests :smirk:

[Tobias-Fischer]

Hi @pattacini and @drdanz,
Shall we go for the quick fix I suggested (as modified by @pattacini)?

Best, Tobi

[pattacini]

What do you think @drdanz ?
It might be reasonable to go for the temporary patch in view of the major overhaul we were discussing above.

[Tobias-Fischer]

Any news @drdanz @pattacini?

[pattacini]

@drdanz something we might want to revive for any upcoming YARP release.