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.

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

Serial::setTimeout should accept param by const reference #101

Open
theidealist opened this issue Jun 3, 2015 · 2 comments
Open

Serial::setTimeout should accept param by const reference #101

theidealist opened this issue Jun 3, 2015 · 2 comments

Comments

@theidealist
Copy link

It's misleading (and inconsistent with the constructor version of the same API) to accept this parameter by non-const reference as it indicates to me the user that it is being "captured" by reference and that I should therefore keep the timeout object I provide it around for at least as long as I have the Serial object itself, or that the object will be modified by the function itself. Neither of these do I believe to be true after closer inspection of the code.

If there is data being changed in Timeout (I followed the pimpl implementation through to unix.cc:702) it should be advertised more clearly in the documentation.

Thanks!

@wjwwood
Copy link
Owner

wjwwood commented Jun 8, 2015

I agree, the setTimeout method should take a const Timeout &. The constructor should probably do the same. I just need to test how the default argument works out in that case.

I'll do this when I get some time, but it would go quicker with a pull request. Also, I'll need to consider how this breaks the ABI/API and then figure out what future release it should be included into.

theidealist added a commit to theidealist/serial that referenced this issue Jun 15, 2015
@theidealist
Copy link
Author

I submitted #102 to hopefully make this go a little faster. I hope it is acceptable.

Thanks for your attention @wjwwood !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants