raffaello-camoriano / RBDemo

Reactive Behaviors Demo
1 stars 0 forks source link

Use high-level C++ objects when possible #6

Closed pattacini closed 10 years ago

pattacini commented 10 years ago

what? Use std::string instead, such as:

std::string receivedCmd = command.get(0).asString().c_str();

Lately, yarp::os::ConstString has been made directly compatible with std::string (i.e. it's basically defined as a std::string), hence the *.c_str() is no longer needed.

pattacini commented 10 years ago

Again... here, use rather: receivedCmd == "open_left_hand"

raffaello-camoriano commented 10 years ago

Fixed, assuming that YARP_CONSTSTRING_IS_STD_STRING == 1.

Should I be more specific and write an if/else over YARP_CONSTSTRING_IS_STD_STRING?

pattacini commented 10 years ago

@Ommac , in your opinion what would be the best solution between the two possibilities?

here: the syntax is wrong

raffaello-camoriano commented 10 years ago

Syntax fixed. Too much matlab.

If I include yarp/os/ConstString, then the following code is executed:

// yarp::os::ConstString == std::string
#ifndef YARP_WRAP_STL_STRING
#include <string>
#define YARP_CONSTSTRING_IS_STD_STRING 1
 namespace yarp {
     namespace os {
         typedef std::string ConstString;
     }
}

And YARP_CONSTSTRING_IS_STD_STRING is set to 1. YARP_WRAP_STL_STRING is defined in yarp/conf/system.h, which I guess is a set of definition which can change according to the user preferences about YARP configuration. My system.h, for instance, defines YARP_WRAP_STL_STRING, so if I include it ConstString would not be equal to std::string.

Solution 1 One solution would be to put the following code at the beginning:

// yarp::os::ConstString == std::string
#ifndef YARP_CONSTSTRING_IS_STD_STRING
#define CONSTSTRING_IS_STD_STRING 0
#else
#define CONSTSTRING_IS_STD_STRING 1

and then perform a check in respond(...) as follows:

string receivedCmd
if (CONSTSTRING_IS_STD_STRING)
receivedCmd = command.get(0).asString();
else
receivedCmd = command.get(0).asString().c_str();

Solution 2 Disregard everything and continue to use receivedCmd = command.get(0).asString().c_str(), which is always compatible.

Solution 1 should be slightly more efficient, since the method c_str() is not called. Solution 2 is the best for simplicity and readability.

Since this is code which runs on a robot, I would give a higher priority to efficiency, so I would say I choose solution 1. Am I not considering something important?

pattacini commented 10 years ago

Readability is also important, since the call to c_str() is practically instantaneous (i.e. it returns a memory location).

The check against CONSTSTRING_IS_STD_STRING tends to be not portable too.

Go for solution 2.

raffaello-camoriano commented 10 years ago

Ok, done & pushed.