[WIP] Yarp types names #2231
[WIP] Yarp types names #2231
Conversation
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update the release notes by adding a file in |
| yarp_type_name = annotations.at("yarp.type.name"); | ||
| } | ||
| else { | ||
| yarp_type_name = "yarp/"+name; |
There was a problem hiding this comment.
I'm not convinced about this default value, I'd rather enable this only if specified by the user (at least for now), or everyone who defined some type in some other repo will have some "yarp" types.
There was a problem hiding this comment.
I see. Better name? or just an empty string?
There was a problem hiding this comment.
Empty string and do not add the type part if not defined
| yarp_type_version = annotations.at("yarp.type.version"); | ||
| } | ||
| else { | ||
| yarp_type_version = "1.0"; |
There was a problem hiding this comment.
Same about the default version, 1.0 is not a good value, since an unstable type with no annotation defined will have a stable version. I'd stick with enabling this part only if explicitly added by the user.
There was a problem hiding this comment.
No, I'd just leave it empty, and add the relative calls when generating the code only if it is defined
|
|
||
| f_h_ << indent_h() << "//The name and the version for this message\n"; | ||
| f_h_ << indent_h() << "static constexpr const char* typeName = \"" << yarp_type_name << "\";\n"; | ||
| f_h_ << indent_h() << "static constexpr const char* typeVersion = \"" << "1.0" << "\";\n"; |
| f_h_ << indent_h() << "yarp::os::Type getType() const;\n"; | ||
| f_h_ << '\n'; | ||
|
|
||
| f_cpp_ << indent_cpp() << "// Convert to a printable string\n"; |
There was a problem hiding this comment.
This comment seems a copy-paste from somewhere else
| f_cpp_ << indent_cpp() << "{\n"; | ||
| indent_up_cpp(); | ||
| { | ||
| f_cpp_ << indent_cpp() << " yarp::os::Type typ = yarp::os::Type::byNameOnWire(typeName);\n"; |
There was a problem hiding this comment.
The extra space before yarp::os::Type (and 2 lines below before return) shouldn't be there
| f_cpp_ << indent_cpp() << "{\n"; | ||
| indent_up_cpp(); | ||
| { | ||
| f_cpp_ << indent_cpp() << " yarp::os::Type typ = yarp::os::Type::byNameOnWire(typeName);\n"; |
There was a problem hiding this comment.
yarp::os::Type::byNameOnWire(typeName) is equivalent to yarp::os::Type::byName("yarp/bottle", typeName). We should definitely investigate what's the meaning of the first string, and if it is actually correct to use byNameOnWire, or it should be yarp::os::Type::byName(typeName, typeName)
drdanz
added
Target: YARP v3.4.0
PR Type: Feat/Enh
| { | ||
| unsigned int major = 0; | ||
| unsigned int minor = 0; | ||
| int ret = sscanf(mPriv->version.c_str(), "%u.%u", &major, &minor); |
There was a problem hiding this comment.
|
|
randaz81
force-pushed
the
master
branch
3 times, most recently
from
71900d2
to
ead1caf
Compare
This PR is not yet complete and its intended for discussion only.
I modified the yarp thrift generator to add additional stuff in the generated code of the yarp data types. This will allow a yarp data type to know the name (string) of its own type, and the message type version.