yarp icon indicating copy to clipboard operation
yarp copied to clipboard

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

Open GiulioRomualdi opened this issue 4 years ago • 8 comments
trafficstars

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):

  • OS: Ubuntu
  • yarp version: Yarp 3.4 b966f9f2978f8b0974487d4edc425a91c9609f7b
  • compiler: gcc9

GiulioRomualdi avatar May 24 '21 16:05 GiulioRomualdi

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 avatar May 24 '21 16:05 GiulioRomualdi

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

GiulioRomualdi avatar May 24 '21 17:05 GiulioRomualdi

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

PeterBowman avatar May 24 '21 17:05 PeterBowman

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.

GiulioRomualdi avatar May 24 '21 17:05 GiulioRomualdi

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).

PeterBowman avatar May 24 '21 17:05 PeterBowman

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.

drdanz avatar May 25 '21 09:05 drdanz

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)");

PeterBowman avatar May 25 '21 11:05 PeterBowman

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 

GiulioRomualdi avatar May 25 '21 17:05 GiulioRomualdi