robotology / yarp

YARP - Yet Another Robot Platform
http://www.yarp.it
Other
531 stars 195 forks source link

`yarp::os::Value::isBool()` returns false even if the value is boolean #2584

Open GiulioRomualdi opened 3 years ago

GiulioRomualdi commented 3 years ago

Describe the bug I'm trying to store several yarp::os::Value inside a yarp::os::Bottle. Before retrieving the object I check the type of the object. When the object stored in yarp::os::Values is boolean yarp::os::Value::isBool() returns false.

To Reproduce

The cpp code follows


#include <string>
#include <iostream>

#include <yarp/os/Value.h>
#include <yarp/os/Bottle.h>

int main(int argc, char **argv) {

    bool parameter = true;
    std::string parameterName = "flag";

    yarp::os::Bottle container;
    yarp::os::Value newVal;
    yarp::os::Bottle* list = newVal.asList();

    list->add(yarp::os::Value(parameterName));
    list->add(yarp::os::Value(parameter));
    container.add(newVal);

    yarp::os::Value* value;
    if (!container.check(parameterName, value))
    {
        std::cerr << "Unable to find the parameter" << std::endl;
        return EXIT_FAILURE;
    }

    if (!value->isBool())
    {
        std::cerr << "The value is not a boolean." << std::endl;
        return EXIT_FAILURE;
    }

    return EXIT_SUCCESS;
};
project(yarp-test)

cmake_minimum_required(VERSION 3.0)
find_package(YARP REQUIRED)

add_executable(yarp-test main.cpp)
target_link_libraries(yarp-test YARP::YARP_os)

The application prints

The value is not a boolean.

Expected behavior value->isBool() should return true

Configuration (please complete the following information):

GiulioRomualdi commented 3 years ago

I just noticed that the error appears also with this simple code


#include <string>
#include <iostream>

#include <yarp/os/Value.h>
#include <yarp/os/Bottle.h>

int main(int argc, char **argv) {

    bool parameter = true;
    yarp::os::Value value = yarp::os::Value(parameter);

    if (!value.isBool())
    {
        std::cerr << "The value is not a boolean." << std::endl;
        return EXIT_FAILURE;
    }

    return EXIT_SUCCESS;
};

Am I doing something wrong?

GiulioRomualdi commented 3 years ago

Accordingly to the documentation, seems there is no way to pass a bool in a Value. Probably the boolean is implicit converted in a int and this constructor is called image

However, now I don't understand when isBool() should return true

PeterBowman commented 3 years ago

It is only true if we store a vocab that is either 0 or equal to '1' (ASCII -> 49): ref.

https://github.com/robotology/yarp/blob/10dc2b5fb6223871b06d67110ac22ba3ea44ad6a/src/libYARP_os/src/yarp/os/impl/Storable.h#L869-L872

GiulioRomualdi commented 3 years ago

Hi @PeterBowman, thank you for the response.

Accordingly to your suggestion I modified the code as follows


#include <string>
#include <iostream>

#include <yarp/os/Value.h>
#include <yarp/os/Bottle.h>

int main(int argc, char **argv) {

    yarp::os::Value value = yarp::os::Value('1');

    if (!value.isBool())
    {
        std::cerr << "The value is not a boolean." << std::endl;
        return EXIT_FAILURE;
    }

    return EXIT_SUCCESS;
};

however, the error is still there.

PeterBowman commented 3 years ago

According to docs, you must pass a second parameter so that the first one is interpreted as a vocab, namely Value('1', true). In any case, I don't think this can be used to reliably store (or pretend to store) a Boolean Value. Ideally, another constructor would be added for proper Boolean support, and a StoreBool be derived from Storable (see inheritance diagram).

drdanz commented 3 years ago

If I remember correctly I tried some time ago, but adding a StoreBool and proper methods that accept bool would cause conflicts in the API and in the bottle serialization protocol... The problem is that at the moment, as @PeterBowman mentioned, a "bool" is a Vocab, that is either 0 or 49 (0x31, i.e. the '1' character). This is probably nonsense, but there is code relying on that.

Note that there is also a lot of code that uses VOCAB_OK and VOCAB_FAIL as boolean, see for example https://github.com/robotology/yarp/blob/10dc2b5fb6223871b06d67110ac22ba3ea44ad6a/src/libYARP_os/src/yarp/os/idl/WireWriter.cpp#L75-L80 and https://github.com/robotology/yarp/blob/10dc2b5fb6223871b06d67110ac22ba3ea44ad6a/src/libYARP_os/src/yarp/os/idl/WireReader.cpp#L101-L119

as a consequence, all idl generated classes use this convention.

This is quite confusing in my opinion, but that's not something we can fix without breaking code, and all binary protocols.

PeterBowman commented 3 years ago

The problem is that at the moment, as @PeterBowman mentioned, a "bool" is a Vocab, that is either 0 or 49 (0x31, i.e. the '1' character). This is probably nonsense, but there is code relying on that.

For further reference, a 0 vocab is interpreted as Boolean false and '1' as Boolean true (ref). Conversely, a "true" or "false" string is parsed as Boolean by the internal Bottle mechanisms if properly triggered, i.e. via Bottle constructor or Property's fromString or Value's makeValue or similar (but not via Value(const std::string&)). In other words, this interpretation is hardcoded in such a way that passing --opt true or --opt false (while reading from .ini file or using the command line) will make isBool() evaluate to true on opt because of the implicit/transparent conversion to a vocab via string parsing under the hood.

@GiulioRomualdi knowing this fact, you can overcome this limitation with strings:

auto * v = yarp::os::Value::makeValue("true"); // or "false"
std::cout << v->isBool() << "\n"; // true
delete v; // sadly, `makeXXX()` static methods return unmanaged raw pointers

Alternatively:

yarp::os::Bottle b("true"); // or "false"
std::cout << b.get(0).isBool() << "\n"; // true

In your first example:

// we invoke `add(Value*)` here instead of `add(const Value&)`, dynamic memory is owned by the containing Bottle
list->add(yarp::os::Value::makeValue("true")); // note `makeValue` only accepts strings

Or just:

yarp::os::Bottle container("(flag true)");
GiulioRomualdi commented 3 years ago

Hi @PeterBowman and @drdanz thank you for your suggestions I will go for

list->add(yarp::os::Value::makeValue("true")); // note `makeValue` only accepts strings