Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly handle 8,16,32,64 bit integers and 32,64 bit floating point values #1677

Merged
merged 28 commits into from
May 22, 2018

Conversation

drdanz
Copy link
Member

@drdanz drdanz commented May 7, 2018

This was supposed to be a rather trivial change, but it ended up in the classic "can of worms"...

The handling of 8,16,32, and 64 bit integers was rather incomplete and there was no way to transfer 8 and 16 bit integers, for example yarpidl_rosmsg was bypassing this by sending 8 and 16 bit as "blocks".

Moreover, all the "Int" and "Double" functions, were not architecture safe, on different architectures, these types could have different size, therefore this might lead to unexpected bugs.

The list of changes is rather long, please check each commit, for details, I'll summarize the most important changes here:

  • Replace YARP_INT{8,16,32,64} with std::int{8,16,32,64}_t from cstdint

  • Use defines from cinttypes instead of YARP_INT64_FMT

  • Add yarp::conf::float{32,64,128}_t (128 only if supported)

  • Add yarp::conf::ssize_t

  • Deprecate YARP_INT{8,16,32,64} in favour of std::int{8,16,32,64}_t

  • Deprecate YARP_FLOAT{32,64} in favour of yarp::conf::float{32,64}_t

  • Deprecate YARP_INT64_FMT in favour of PRId64

  • Deprecate YARP_CONF_SSIZE_T in favour of yarp::conf::ssize_t

  • Add the following methods:

    • yarp::os::Bottle::addInt{8,16,32,64}()
    • yarp::os::Bottle::addFloat{32,64}()
    • yarp::os::Value::{is,as,make}Int{8,16,32,64}()
    • yarp::os::Value::{is,as,make}Float{32,64}()
    • yarp::os::ConnectionWriter::appendInt{8,16,32,64}()
    • yarp::os::ConnectionWriter::appendFloat{32,64}()
    • yarp::os::ConnectionReader::expectInt{8,16,32,64}()
    • yarp::os::ConnectionReader::expectFloat{32,64}()
    • BOTTLE_TAG_INT{8,16,32,64} (BOTTLE_TAG_INT32 = BOTTLE_TAG_INT)
    • BOTTLE_TAG_FLOAT{32,64} (BOTTLE_TAG_FLOAT64 = BOTTLE_TAG_DOUBLE)
  • The following methods are not deprecated due to the large codebase where they are used, but their usage in new code is discouraged, and it is forbidden inside YARP. Anyway, all these methods are now calling the corresponding Int32 and Float64 version, but are not architecture safe, since they still accept int and double.

    • yarp::os::Bottle::addInt()
    • yarp::os::Bottle::addDouble()
    • yarp::os::Value::isInt()
    • yarp::os::Value::isDouble()
    • yarp::os::Value::asInt()
    • yarp::os::Value::asDouble()
    • yarp::os::Value::makeInt()
    • yarp::os::Value::makeDouble()
    • yarp::os::ConnectionWriter::appendInt()
    • yarp::os::ConnectionWriter::appendDouble()
    • yarp::os::ConnectionReader::expectInt()
    • yarp::os::ConnectionReader::expectDouble()
    • BOTTLE_TAG_INT
    • BOTTLE_TAG_DOUBLE

WARNING - Behaviour change: An Int64 value no longer returns true when isInt() is called. I'm not sure if this was an intended behaviour or a bug, but it does not make sense.

I added a "fake" unit test, whose only goal is to print this table, that is really useful for debugging.

screenshot_20180507_182639

In a second moment, it could be useful to extend this table with the other types supported by Value


Edit:

  • yarp::os::Value ctors are now explicit
  • Added ResourceFinder::setDefault() overloads for int32_t and float64_t.

@drdanz drdanz added PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP Component: Library - YARP_os Status: In Progress Type: Cleanup Involves cleaning up some part of YARP PR Status: Changelog - Not OK This PR does not have a changelog entry, or the changelog needs to be fixed PR Type: Bugfix This PR fixes some bug Type: Refactor Involves refactoring some part of code PR Status: Test on Hardware - Not OK The code in this PR needs to be tested on the robot setup Type: Breaking/Behaviour Change Involves breaking user code or behaviour Target: YARP v3.0.0 labels May 7, 2018
@drdanz drdanz self-assigned this May 7, 2018
@drdanz drdanz added this to To do in YARP v3.0.0 (2018-06-11) via automation May 7, 2018
@drdanz drdanz force-pushed the YARP_INT branch 2 times, most recently from 8260ce0 to 928e2a4 Compare May 8, 2018 17:40
drdanz added a commit to drdanz/icub-main that referenced this pull request May 11, 2018
Due to robotology/icub-firmware-shared#22 defining float32_t,
conflicting with YARP yarp::conf::float32_t introduced in
robotology/yarp#1677, icub-firmware-shared headers must be included
before any include from YARP, since doing the opposite will cause
a build error.
drdanz added a commit to drdanz/icub-main that referenced this pull request May 11, 2018
Due to robotology/icub-firmware-shared#22 defining float32_t,
conflicting with YARP yarp::conf::float32_t introduced in
robotology/yarp#1677, icub-firmware-shared headers must be included
before any include from YARP, since doing the opposite will cause
a build error.
@drdanz drdanz force-pushed the YARP_INT branch 2 times, most recently from 72b8c1b to cef348d Compare May 14, 2018 14:46
@drdanz drdanz added PR Status: Changelog - OK This PR has a proper changelog entry and removed PR Status: Changelog - Not OK This PR does not have a changelog entry, or the changelog needs to be fixed labels May 14, 2018
@drdanz drdanz added PR Status: Test on Hardware - OK The code in this PR was successfully tested on the robot setup and removed PR Status: Test on Hardware - Not OK The code in this PR needs to be tested on the robot setup labels May 21, 2018
@drdanz drdanz merged commit 5313b8e into robotology:devel May 22, 2018
YARP v3.0.0 (2018-06-11) automation moved this from In progress to Done May 22, 2018
@drdanz drdanz deleted the YARP_INT branch May 22, 2018 08:42
Nicogene added a commit to Nicogene/yarp that referenced this pull request May 25, 2018
Nicogene added a commit to Nicogene/yarp that referenced this pull request May 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Library - YARP_os PR Status: Changelog - OK This PR has a proper changelog entry PR Status: Test on Hardware - OK The code in this PR was successfully tested on the robot setup PR Type: Bugfix This PR fixes some bug PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP Resolution: Merged Target: YARP v3.0.0 Type: Breaking/Behaviour Change Involves breaking user code or behaviour Type: Cleanup Involves cleaning up some part of YARP Type: Refactor Involves refactoring some part of code
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

1 participant