Make Vocab constexpr by Nicogene · Pull Request #1764 · robotology/yarp

Nicogene · 2018-06-21T13:38:14Z

This PR makes vocab constexpr.

Merge this PR after : robotology/robot-testing-framework#98

Please review code

PeterBowman · 2018-06-21T14:06:44Z

Idea: remove VOCABX, use default parameters: constexpr int32_t VOCAB(char a, char b = 0, char c = 0, char d = 0) { ... }. Regarding the implementation, you probably don't need so many parentheses as in a macro, perhaps even the (int) casts could be left out (not sure though, and BTW: shouldn't these be int32_t otherwise, for consistency?).

Nicogene · 2018-06-21T15:29:57Z

Idea: remove VOCABX, use default parameters: constexpr int32_t VOCAB(char a, char b = 0, char c = 0, char d = 0) { ... }
@PeterBowman you are right, this another reason to move to constexpr

PeterBowman · 2018-06-21T16:39:22Z

Another idea (which I'm not quite convinced of): VOCAB('a', 'b', 'c', 'd') (global scope) or yarp::os::VOCAB('a', 'b', 'c', 'd') (C++ namespace)?

drdanz · 2018-06-21T16:50:37Z

or maybe yarp::os::makeVocab('a', 'b', 'c', 'd'), and leave VOCAB in global scope, but deprecated

drdanz · 2018-06-21T17:09:11Z

And by the way, it would be nice to have also a "string" version

yarp::os::makeVocab("abcd");

drdanz · 2018-06-21T17:35:25Z

Actually there is a yarp::os::encode(const std::string&) already... that should become constexpr as well.

jgvictores

In its current state, the PR introduces some issues regarding SWIG bindings:

Regression on VOCABs defined in the global scope: The changes introduced at #1696 have been removed. Even with an advanced version of SWIG, there is no generation of VOCABs defined in the global scope required by interfaces (e.g. no VOCAB_CM_POSITION). Not caught on Travis because of used SWIG version
Recovering some of the bindings/yarp.i logic and the preprocessor protection should suffice. 😄
Malformed results via wrapped new makeVocab function:

In [1]: yarp.encode("abcd")
Out[1]: 1684234849

In [2]: yarp.makeVocab('a','b','c','d')
Out[2]: <Swig Object of type 'int32_t *' at 0x7f95d9443840>

Not caught on Travis because it's new. Could definitely be added in Lua tests at tests/test_vocab.lua! 😃

Nicogene · 2018-06-23T08:06:27Z

Thanks @jgvictores for checking the bindings part, I wasn't sure about the changes, but I thought that if the compiler and the test doesn't complain is ok 😅

About the 1.: createVocab takes place of VOCAB and it is now scoped, these changes you made are still necessary?

PeterBowman · 2018-06-26T15:35:57Z

+// We need a constexpr for efficient switching.
+// Use as, for example, makeVocab('s','e','t')
+constexpr yarp::conf::vocab32_t createVocab(char a, char b = 0, char c = 0, char d = 0)  {
+    return ((((yarp::conf::vocab32_t)(d))<<24)+(((yarp::conf::vocab32_t)(c))<<16)+(((yarp::conf::vocab32_t)(b))<<8)+((yarp::conf::vocab32_t)(a)));


A bit cleaner:

return ((yarp::conf::vocab32_t)d << 24) + ((yarp::conf::vocab32_t)c << 16) + ((yarp::conf::vocab32_t)b << 8) + (yarp::conf::vocab32_t)a;

I'd remove those parentheses around each parameter, the final return, and each C-style cast (ref), here and at VOCABX below, as an unnecessary remnant of the former implementation via C preprocessor macros.

What would you think about c++-style casts, which are more explicit?

Also I'd really appreciate if you could expand this on more than one line, and add them in the proper order (i.e. a-b-c-d, not d-c-b-a) in order to make it more readable, like here:

https://github.com/robotology-playground/sharedlibpp/blob/45b5cae5b50c6431e5eae8bb8aebf6d751054f8b/src/shlibpp/SharedLibraryClassApi.h#L63:L67

PeterBowman · 2018-06-26T18:33:46Z

+YARP_DEPRECATED_MSG("Use yarp::os::createVocab() instead")
+constexpr int32_t VOCAB2(char a, char b)                 { return yarp::os::createVocab((a),(b),(0),(0)); }
+YARP_DEPRECATED_MSG("Use yarp::os::createVocab() instead")
+constexpr int32_t VOCAB1(char a)                         { return yarp::os::createVocab((a),(0),(0),(0)); }


Same as #1764 (comment): yarp::os::createVocab(a,b,c,d).

Moreover: - `Vocab` is now a namespace. - `vocab32_t` has been defined in yarp/conf/numeric.h

Moreover change test_vocab.lua to make it work after the changes in Vocab and add an additional test.

drdanz · 2018-06-27T08:54:24Z

+YARP_DEPRECATED_MSG("Use yarp::os::createVocab() instead")
+constexpr int32_t VOCAB2(char a, char b)                 { return yarp::os::createVocab(a,b,0,0); }
+YARP_DEPRECATED_MSG("Use yarp::os::createVocab() instead")
+constexpr int32_t VOCAB1(char a)                         { return yarp::os::createVocab(a,0,0,0); }


I think these methods should be returning yarp::conf::vocab32_t as well

Nicogene added PR Type: Feat/Enh This PR adds some new feature or enhances some part of YARP Component: Library - YARP_os Component: RTF Plugins PR Status: Changelog - Not Required This PR does not need a changelog entry Target: YARP v3.1.0 labels Jun 21, 2018

Nicogene self-assigned this Jun 21, 2018

Nicogene requested a review from drdanz June 21, 2018 13:38

Nicogene mentioned this pull request Jun 21, 2018

Make vocab constexpr robotology/robot-testing-framework#98

Merged

Nicogene force-pushed the vocabConstExpr branch from 5e74b0e to d06e77b Compare June 21, 2018 16:09

Nicogene requested review from barbalberto, randaz81 and traversaro as code owners June 21, 2018 16:09

Nicogene added PR Status: Branch - OK This PR is targetting the right branch PR Status: Changelog - OK This PR has a proper changelog entry and removed PR Status: Changelog - Not Required This PR does not need a changelog entry labels Jun 21, 2018

Nicogene force-pushed the vocabConstExpr branch from d06e77b to b1f6c1c Compare June 21, 2018 17:19

Nicogene force-pushed the vocabConstExpr branch 2 times, most recently from 6b6de85 to ed43a28 Compare June 22, 2018 13:33

jgvictores suggested changes Jun 22, 2018

View reviewed changes

Nicogene force-pushed the vocabConstExpr branch from ed43a28 to 6f5d5f4 Compare June 23, 2018 09:02

Nicogene added Component: Bindings swig, python, java, ruby, perl, octave, matlab, lua, csharp, tcl Component: Library - YARP_dev Component: Devices labels Jun 23, 2018

PeterBowman reviewed Jun 26, 2018

View reviewed changes

Nicogene force-pushed the vocabConstExpr branch 2 times, most recently from 46b9edd to be976de Compare June 26, 2018 17:11

PeterBowman reviewed Jun 26, 2018

View reviewed changes

Nicogene added 17 commits June 26, 2018 22:12

libYARP_OS: make VOCABs c++11 constexpr.

0303d86

Moreover: - `Vocab` is now a namespace. - `vocab32_t` has been defined in yarp/conf/numeric.h

YARP_dev: add generic vocab header

b073f8a

Fix compilation warning in NameConfig

e3a744f

Increase RTF version required

0af4729

carriers: change VOCABx deprecated to createVocab

f4f3caa

devices: change VOCABx deprecated to createVocab

ead5c55

thrift: change VOCABx deprecated to createVocab

c04641c

YARP_OS: change VOCABx deprecated to createVocab.

506737e

YARP_companion: change VOCABx deprecated to createVocab.

6990da2

YARP_dev: change VOCABx deprecated to createVocab.

0b2ab68

YARP_serserversql: change VOCABx deprecated to createVocab.

c0702e9

YARP_sig: change VOCABx deprecated to createVocab.

ced2c38

YARP_wire_rep_utils: change VOCABx deprecated to createVocab.

90ca19c

tests: change VOCABx deprecated to createVocab.

01a1dff

Update release notes

0f7d7dc

Bindings: add changes necessary after making vocab a constexpr.

537e235

Moreover change test_vocab.lua to make it work after the changes in Vocab and add an additional test.

example: change VOCABx deprecated to createVocab.

1cfddf5

Nicogene force-pushed the vocabConstExpr branch from be976de to 1cfddf5 Compare June 26, 2018 20:18

traversaro approved these changes Jun 26, 2018

View reviewed changes

Nicogene merged commit fbe606f into robotology:devel Jun 27, 2018

Nicogene added the Resolution: Merged label Jun 27, 2018

drdanz reviewed Jun 27, 2018

View reviewed changes

Nicogene removed their assignment Jun 27, 2018

PeterBowman mentioned this pull request Jul 6, 2018

Locally (re)defined VOCABs are no longer compatible with YARP devel roboticslab-uc3m/kinematics-dynamics#158

Closed

Nicogene mentioned this pull request Jul 10, 2018

YARP_dev: make INavigation2D a derived interface of ILocalization2D. #1788

Merged

Nicogene deleted the vocabConstExpr branch July 10, 2018 14:09

Conversation

Nicogene commented Jun 21, 2018

Uh oh!

PeterBowman commented Jun 21, 2018

Uh oh!

Nicogene commented Jun 21, 2018

Uh oh!

PeterBowman commented Jun 21, 2018

Uh oh!

drdanz commented Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drdanz commented Jun 21, 2018

Uh oh!

drdanz commented Jun 21, 2018

Uh oh!

jgvictores left a comment

Choose a reason for hiding this comment

Uh oh!

Nicogene commented Jun 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PeterBowman Jun 26, 2018

Choose a reason for hiding this comment

Uh oh!

jgvictores Jun 26, 2018

Choose a reason for hiding this comment

Uh oh!

drdanz Jun 26, 2018

Choose a reason for hiding this comment

Uh oh!

PeterBowman Jun 26, 2018

Choose a reason for hiding this comment

Uh oh!

drdanz Jun 27, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

drdanz commented Jun 21, 2018 •

edited

Loading

Nicogene commented Jun 23, 2018 •

edited

Loading