feat: 🧑💻 Check device literal port numbers at compile time #689
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Please fix this MR to only include relevant commits so its reviewable. |
ion098
force-pushed
the
feature/comptime-port-check
branch
from
793eac0 to
9a792f0
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ion098
changed the title
ion098
changed the title
| // from_chars doesn't seem to handle hex/binary/octal prefixes, | ||
| // so we handle them ourselves |
There was a problem hiding this comment.
This sucks lmao, good catch though
There was a problem hiding this comment.
Turns out it does, I failed to read the docs lol. Fixed in bea9c44.
There was a problem hiding this comment.
Turns out I failed to read the docs doubly lol: from_chars does indeed NOT auto detect base womp womp
There was a problem hiding this comment.
lol? so what, we need to add back in the 0b and 0x detection? that sucks bruh
| // hex literal | ||
| base = 16; |
There was a problem hiding this comment.
Does std::from_chars handle both uppercase and lowercase hex literals?
include/pros/distance.hpp
Outdated
| const pros::Distance operator"" _dist(const unsigned long long int d); | ||
| template <char... Cs> | ||
| const pros::Distance operator"" _dist() { | ||
| static_assert(pros::detail::is_valid_port<Cs...>(), "Port is out of range!"); |
There was a problem hiding this comment.
Was originally unsure if this is better than requires-clause, but it probably is because it allows giving a message (and benefits of requires-clause, ie SFINAE, arent really useful here).
The message you have is not great though, because a failed from_chars will give the same error. I imagine that this happens if you try to give it a non-integer? I would probably prefer "invalid port number" or something
There was a problem hiding this comment.
Same goes for other declarations
| @@ -2419,7 +2426,12 @@ const pros::Motor operator"" _mtr(const unsigned long long int m); | |||
| * } | |||
| * \endcode | |||
| */ | |||
| const pros::Motor operator"" _rmtr(const unsigned long long int m); | |||
| template <char... Cs> | |||
| const pros::Motor operator"" _rmtr() { | |||
There was a problem hiding this comment.
I wonder if theres a single pros project in the whole world that uses _rmtr
There was a problem hiding this comment.
| /** | ||
| * Returns a rotation sesnor that has the same port as the original, but reversed. | ||
| * | ||
| * This can only be used on a temporary object (such as a device literal). | ||
| * | ||
| * \b Example | ||
| * \code | ||
| * using namespace pros::literals; | ||
| * | ||
| * void opcontrol() { | ||
| * pros::Rotation rotation_sensor = -1_rot; | ||
| * printf("Reversed: %d \n", rotation_sensor.get_reversed()); | ||
| * } | ||
| * \endcode | ||
| */ | ||
| Rotation operator-() && { return Rotation(-this->get_port()); } |
There was a problem hiding this comment.
why should this exist? can you not create negatives with the other user-defined literal operator? If you can, then this probably shouldnt exist
There was a problem hiding this comment.
Oh I see that its rvalue-this-qualified. still a little weird
There was a problem hiding this comment.
No, not for rotation sensors. Both 10_rrot and 10_rotr seem a bit confusing compared to -10_rot (negative port numbers also match how the ctors work)
There was a problem hiding this comment.
If this is necessary for rotation, its surely necessary for motors too?
There was a problem hiding this comment.
hopefully its not necessary for either though lol
There was a problem hiding this comment.
im just confused why this doesnt work with the user-defined literal directly.
There was a problem hiding this comment.
Because -10_rot is -(operator""_rot<'1','0'>()), not operator""_rot<'-','1','0'>()
There was a problem hiding this comment.
Motors have _rmtr so its not needed for motors, although adding this to motors might be a good idea for consistency
There was a problem hiding this comment.
oh i see. yeah that sucks lmao. i think this should be added to motors as well if it has to exist.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@Rocky14683 can you rerun the pipeline please? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@Rocky14683 It should compile now, can you rerun the pipeline please? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ion098
changed the title
Summary:
Checks port numbers for device literals at compile time rather than at runtime. (Thanks to @djava for giving me suggestions on refactoring my mess of templates into actually readable code)
Motivation:
If something can be checked at compile time, it should be checked at compile time.
Test Plan:
Note:
This is an ABI breaking change and therefore should be a minor version bump.