Better std.algorithm.comparison.clamp error messages #8717
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
burner
requested review from
JackStouffer,
PetarKirov,
andralex and
wilzbach
as code owners
Contributor
|
Thanks for your pull request, @burner! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#8717" |
RazvanN7
requested changes
Mar 14, 2023
changelog/clamp_assert_message.dd
Outdated
| @@ -0,0 +1,8 @@ | |||
| Better static assert messages for `std.algorithm.comparion.clamp` | |||
Collaborator
There was a problem hiding this comment.
Suggested change
| Better static assert messages for `std.algorithm.comparion.clamp` | |
| Better static assert messages for `std.algorithm.comparison.clamp` |
changelog/clamp_assert_message.dd
Outdated
| @@ -0,0 +1,8 @@ | |||
| Better static assert messages for `std.algorithm.comparion.clamp` | |||
|
|
|||
| Up until now `clamp` used a template constraint to check if the passed types | |||
Collaborator
There was a problem hiding this comment.
Suggested change
| Up until now `clamp` used a template constraint to check if the passed types | |
| Up until now, `clamp` used a template constraint to check if the passed types |
changelog/clamp_assert_message.dd
Outdated
| Better static assert messages for `std.algorithm.comparion.clamp` | ||
|
|
||
| Up until now `clamp` used a template constraint to check if the passed types | ||
| could be used. If it were not, it was very tedious to figure out why. |
Collaborator
There was a problem hiding this comment.
Suggested change
| could be used. If it were not, it was very tedious to figure out why. | |
| could be used. If they were not, it was very tedious to figure out why. |
changelog/clamp_assert_message.dd
Outdated
| Up until now `clamp` used a template constraint to check if the passed types | ||
| could be used. If it were not, it was very tedious to figure out why. | ||
|
|
||
| As the template constraint is not used to overload the symbol template |
Collaborator
There was a problem hiding this comment.
Suggested change
| As the template constraint is not used to overload the symbol template | |
| As the template constraint is not used for overload resolution |
changelog/clamp_assert_message.dd
Outdated
| could be used. If it were not, it was very tedious to figure out why. | ||
|
|
||
| As the template constraint is not used to overload the symbol template | ||
| function, the constrains are move into static asserts with expressive error |
Collaborator
There was a problem hiding this comment.
Suggested change
| function, the constrains are move into static asserts with expressive error | |
| the constraints are moved into static asserts with expressive error |
| // cannot use : | ||
| // `if (is(typeof(val.lessThan(lower) ? lower : val.greaterThan(upper) ? upper : val) : T1)) | ||
| // because of https://issues.dlang.org/show_bug.cgi?id=16235. | ||
| // Once that is fixed, we can simply use the ternary in both the template constraint |
Collaborator
There was a problem hiding this comment.
This comment is not accurate anymore if the constraint is removed.
std/algorithm/comparison.d
Outdated
| ? lower | ||
| : val.greaterThan(upper) | ||
| ? upper | ||
| : val))); |
Collaborator
There was a problem hiding this comment.
I am confused as to why this assert is used given that no error message is provided.
burner
force-pushed
the
clamp_static_assert_message
branch
from
94e31ff to
258e7ff
Compare
Member
Author
|
@RazvanN7 awesome review thank you, arguable better than the PR itself. |
RazvanN7
reviewed
Mar 15, 2023
Collaborator
RazvanN7
left a comment
RazvanN7
left a comment
There was a problem hiding this comment.
Thanks. A few nits and we are good to go.
| // `if (is(typeof(val.lessThan(lower) ? lower : val.greaterThan(upper) ? upper : val) : T1)) | ||
| // because of https://issues.dlang.org/show_bug.cgi?id=16235. | ||
| // Once that is fixed, we can simply use the ternary in both the template constraint | ||
| // and the template body |
Collaborator
There was a problem hiding this comment.
Please keep this comment. Once the bug is fixed, the body of the function (lines 590-594) can be replaced by a simple ternary.
std/algorithm/comparison.d
Outdated
| in | ||
| { | ||
| static assert(is(T2 : T1), "T2 of type '", T2.stringof | ||
| , "' must be implicitly convertable to type of T1 '" |
Collaborator
There was a problem hiding this comment.
Suggested change
| , "' must be implicitly convertable to type of T1 '" | |
| , "' must be implicitly convertible to type of T1 '" |
Collaborator
There was a problem hiding this comment.
std/algorithm/comparison.d
Outdated
| , "' must be implicitly convertable to type of T1 '" | ||
| , T1.stringof, "'"); | ||
| static assert(is(T3 : T1), "T3 of type '", T3.stringof | ||
| , "' must be implicitly convertable to type of T1 '" |
Collaborator
There was a problem hiding this comment.
Suggested change
| , "' must be implicitly convertable to type of T1 '" | |
| , "' must be implicitly convertible to type of T1 '" |
burner
force-pushed
the
clamp_static_assert_message
branch
from
258e7ff to
d8c627e
Compare
RazvanN7
approved these changes
Mar 16, 2023
Member
Author
|
@RazvanN7 thank you |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.