search

adafruit / Adafruit-PWM-Servo-Driver-Library

Adafruit PWM Servo Driver Library
Other
477 stars 311 forks source link

Change constructor arguments #75

Closed arduino12 closed 1 year ago

arduino12 commented 4 years ago

Hi,

I have 2 minor improvments to the constructors:

  1. Please remove the const attribute from the address argument because it is not needed.
  2. Please replace the TwoWire &i2c with TwoWire *i2c - this makes sense because you save this as a pointer here anyway.

Thanks!

sticilface commented 4 years ago

see this https://github.com/adafruit/Adafruit-PWM-Servo-Driver-Library/issues/50.

the hex addresses used by i2c can be interpreted as a pointer, this caused me significant trouble to get to the bottom of (when the constructor order was changed) and can be totally avoided if the wire instance is passed by reference as it is typed checked. so it is safer and less buggy. even if it is stored as a pointer eventually.

arduino12 commented 4 years ago

Hi,

I didn't know that the constructor order was changed, But I still think my improvments are the right thing to do!

I know many libraries that uses pointers as arguments- If the user pass a wrong value- and ignore the compiler warnings - it is his problem- And only by fixing it he will learn to avoid it next time!

Because I2C slave address values can be 1-127, And in our case they start from 0x40 so only 64 valid address. I can suggest to check the address argument it in the constructors and raise INVALID_ADDRESS error to the user if needed.

positron96 commented 3 years ago

Hi. I vote against removing const and turning reference to a pointer.

While in case of address, const does not improve much, it's still a good practice to const stuff that does not change. For the sake of contract, architecture, code safety, etc. https://softwareengineering.stackexchange.com/questions/332820/in-c-c-should-i-use-const-in-parameters-and-local-variables-when-possible

Same goes for pointers. Why would one use pointers when you can use references? References give more guarantees for the callee side (for starters, references cannot be null).

arduino12 commented 3 years ago

So why all the other functions of this and many many other libs does not use const before every argument..? I still think it is not needed here..

About pointer VS references to Wire, @sticilface convinced me that the references is safer in that case - Thanks for that :)

caternuson commented 1 year ago

Closing. Seems to be just based on code inspection. If there is a demonstrable issue, please post example code and details and can re-open to investigate.