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.

Sign up for GitHub

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

[WIP] Add setExternal in ManagedBytes #1582

Closed
wants to merge 6 commits into from
Closed

[WIP] Add setExternal in ManagedBytes #1582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9fd7c2b
VectorOf.h: add implementation of read / write calling vectorBase fun…
Feb 2, 2016
fd13e59
libYARP_OS: add `setExternal` in `ManagedBytes`.
Nicogene Mar 9, 2018
02f481f
tests: enable `VectorOfTest`
Nicogene Mar 9, 2018
e20c77f
Update release notes
Nicogene Mar 9, 2018
1607336
VectorOfTest: cleanup, removes unused classes
Nicogene Mar 13, 2018
b9ee051
VectorOfTest: fix checkSendReceiveInt test.
Nicogene Mar 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/release/v3_0_0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ Important Changes

* The `shmem` carrier is no longer builtin inside `YARP_OS` and it is now a
plugin.
* Added `setExternal` method in `yarp::os::ManagedBytes`.

#### `YARP_dev`

Expand Down
7 changes: 7 additions & 0 deletions src/libYARP_OS/include/yarp/os/ManagedBytes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,13 @@ class YARP_OS_API yarp::os::ManagedBytes : public Portable {
return owned;
}

/**
* @brief Use this to wrap an external blob of data.
* @param extData, pointer to the external data.
* @param len, size in bytes of the external blob
*/
void setExternal(const char* extData, size_t len);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 comments here.
First of all, this should not be const char*, but just char, since the data is not owned, but can be modified. Second, I suggest to use const Bytes& ext instead, like in the other constructor, in order to make the interface more coherent

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point but I tried to be as much coherent as possible to yarp::sig::Image::setExternal:

yarp/src/libYARP_sig/include/yarp/sig/Image.h

Lines 247 to 252 in 29e9108

/**
* Use this to wrap an external image.
* Make sure to that pixel type and padding quantum are
* synchronized (you can set these in the FlexImage class).
*/
void setExternal(const void *data, int imgWidth, int imgHeight);

What is the best option ? 😅


private:
Bytes b;
bool owned;
Expand Down
13 changes: 13 additions & 0 deletions src/libYARP_OS/src/ManagedBytes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,3 +179,16 @@ bool ManagedBytes::write(ConnectionWriter& writer) {
writer.convertTextMode();
return !writer.isError();
}

void ManagedBytes::setExternal(const char *extData, size_t len)
{
if (!extData)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the style used in the rest of the code if (...) {

return;
}
clear();
owned = false;
Bytes extb(const_cast<char*>(extData), len);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering why setExternal accepts const and if this is safe...
The Bytes b is not const, therefore the const char can be modified later by the user by using Vector methods.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be actually the trieckiest part of this PR. The Vector does not check if it owns the memory (with e.g. a bool owned like ManagedBytes) and in fact it should not do it. We must understand whether there is a risk of memory modification or not when the memory is not owned.

b = extb;
setUsed(len);
}
25 changes: 21 additions & 4 deletions src/libYARP_sig/include/yarp/sig/Vector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,24 @@ class yarp::sig::VectorOf : public VectorBase
len = 0;
first = nullptr;
}

/**
* Read vector from a connection.
* return true if a vector was read correctly
*/
virtual bool read(yarp::os::ConnectionReader& connection) override
{
return VectorBase::read(connection);
}

/**
* Write vector to a connection.
* return true if a vector was written correctly
*/
virtual bool write(yarp::os::ConnectionWriter& connection) override
{
return VectorBase::write(connection);
}
};


Expand Down Expand Up @@ -457,16 +475,15 @@ class YARP_sig_API yarp::sig::Vector : public yarp::os::Portable
void clear ()
{ storage.clear();}

///////// Serialization methods
/*
/**
* Read vector from a connection.
* return true iff a vector was read correctly
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://en.wikipedia.org/wiki/Iff

* @return true if a vector was read correctly
*/
virtual bool read(yarp::os::ConnectionReader& connection) override;

/**
* Write vector to a connection.
* return true iff a vector was written correctly
* @return true if a vector was written correctly
*/
virtual bool write(yarp::os::ConnectionWriter& connection) override;

Expand Down
2 changes: 2 additions & 0 deletions tests/libYARP_sig/TestList.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ namespace yarp {
// method.
extern yarp::os::impl::UnitTest& getImageTest();
extern yarp::os::impl::UnitTest& getVectorTest();
extern yarp::os::impl::UnitTest& getVectorOfTest();
extern yarp::os::impl::UnitTest& getSoundTest();
extern yarp::os::impl::UnitTest& getMatrixTest();

Expand All @@ -33,6 +34,7 @@ class yarp::sig::impl::TestList {
yarp::os::impl::UnitTest& root = yarp::os::impl::UnitTest::getRoot();
root.add(getImageTest());
root.add(getVectorTest());
root.add(getVectorOfTest());
root.add(getMatrixTest());
root.add(getSoundTest());
}
Expand Down
Loading