Closed dogoepp closed 8 years ago
It's good, but I'm not convinced by this part:
default:
os_baudrate = B1000000;
std::stringstream err_message;
err_message << "Invalid baudrate: " << os_baudrate << "! Setting to default: 1,000,000 bauds";
throw Error(err_message.str());
When you throw an Exception, you don't get a return value, so it's useless to set os_baudrate, and the message of setting it to default isn't true.
Right, thanks. I had changed from a ros message and forgot this.
Le 29/01/2016 15:27, Federico Allocati a écrit :
It's good, but I'm not convinced by this part:
|default: os_baudrate = B1000000; std::stringstream err_message; err_message << "Invalid baudrate: " << os_baudrate << "! Setting to default: 1,000,000 bauds"; throw Error(err_message.str()); |
When you throw an Exception, you don't get a return value, so it's useless to set os_baudrate, and the message of setting it to default isn't true.
— Reply to this email directly or view it on GitHub https://github.com/resibots/libdynamixel/pull/4#issuecomment-176780876.
Another question. Any reason why setting os_baudrate in the switch, and then returning it, instead of returning it directly in the swicht statement?
No good reason. I had also though of this option. Do you think it's better ?
Le 29/01/2016 15:52, Federico Allocati a écrit :
Another question. Any reason why setting os_baudrate in the switch, and then returning it, instead of returning it directly in the swicht statement?
— Reply to this email directly or view it on GitHub https://github.com/resibots/libdynamixel/pull/4#issuecomment-176793645.
I just think it's cleaner. Performance-wise, I guess the compiler realises it's the same, doesn't reserve space in the stack for the variable, and instead does return value optimization.
Anyway it's going to be replaced by v2 very soon.
@jbmouret any remark on this ?
Just merge it.. V2 is almost complete and it doesn't change anything..
Shall I merge ?