purduesigbots / pros

Source code for PROS kernel: open source C/C++ development for the VEX V5 microcontroller
https://pros.cs.purdue.edu
Other
259 stars 76 forks source link

✨ PROS 3.8: Add additional ctors to pros::Motor_Group, refine existing ctors #470

Closed djava closed 1 year ago

djava commented 1 year ago

Summary:

  1. Adds the following functions: a. Motor_Group::Motor_Group(const std::vector<pros::Motor>& motors) b. Motor_Group::Motor_Group(const std::initializer_list<std::int8_t> motor_ports)
  2. Removes the explicit tag from the Motor_Group(const std::initializer_list<Motor>) ctor
  3. Changes the Motor_Group(const std::vector<std::int8_t>) ctor to Motor_Group(const std::vector<std::int8_t>&) (passes by const-ref instead of by value)
  4. Changes the definition of the Motor_Group(const std::vector<std::int8_t>&) to use a range-based for loop and std::vector::emplace_back instead of std::vector::push_back

Motivation:

  1. a. Allowing the user to pass a std::vector<Motor>& removes the restriction that Motor_Groups could only be constructed with literal brace-enclosed lists of Motors that existed because std::initializer_list is designed to exist only as an rvalue. Now the options for constructing a Motor_Group with Motors are far more flexible. b. Allows for a more efficient construction of Motor_Groups using a literal brace-enclosed list of port numbers than only accepting std::vector<uint8_t> for ports
  2. I see no real reason that a brace-enclosed list of Motor shouldn't be implicitly convertible to Motor_Group. This change seems like it could create more ergonomic function calls in places, without any real downsides.
  3. Avoids the copy, just more efficient and idiomatic with no downsides.
  4. emplace_back(port) constructs the Motors in-place inside the vector instead of creating temporary Motors and copying them into the vector, as push_back(Motor(port)) does. Also uses range-based for loop to be more idiomatic.
References (optional):

Original inspiration for 1: https://discord.com/channels/197777408198180864/198658294007463936/1044109726675390544

Test Plan:

WillXuCodes commented 1 year ago

/azp run

azure-pipelines[bot] commented 1 year ago
Azure Pipelines successfully started running 1 pipeline(s).
djava commented 1 year ago

Done

Richard-Stump commented 1 year ago

@djava We don't think this can get merged due to breaking changes in the constructors. image The constructors that take vectors have been changed to take references to vectors, and the constructor that takes an initializer list of motors is now non-explicit.

These changes are good fine PROS 4, but we cannot accept these into PROS 3 since they break the ABI. This will break any templates that are not compiled against newer kernels.

djava commented 1 year ago

I will revert that one to passing by value

Richard-Stump commented 1 year ago

I will revert that one to passing by value

Thank you. We will make them references in PROS 4 though.

djava commented 1 year ago

That was a lot of commits to do that 🥴 oops

Richard-Stump commented 1 year ago

Looks good! I'm still trying to figure out if the explicit keyword breaks ABI or not.

Richard-Stump commented 1 year ago

I will try to apply this kernel to an out of date template to ensure that these changes aren't ABI breaking.

djava commented 1 year ago

Looks good! I'm still trying to figure out if the explicit keyword breaks ABI or not.

I'm not certain but I find it hard to imagine that it would

Richard-Stump commented 1 year ago

Yeah, I don't imagine it would, but we didn't think that switching the motor's constructors to signed would break much, but it did. I would just rather be safe than sorry so that we don't repeat that mistake.