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

Check for sortedness must not use == #7928

Merged
merged 4 commits into from
Apr 14, 2021

Conversation

a-ludi
Copy link
Contributor

@a-ludi a-ludi commented Mar 31, 2021

Checking sortedness according to a given predicate must not use == operator
because it may not be in accordance with the predicate.

@dlang-bot
Copy link
Contributor

dlang-bot commented Mar 31, 2021

Thanks for your pull request and interest in making D better, @a-ludi! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
21810 minor Check for sortedness in merge must not use ==

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7928"

@RazvanN7
Copy link
Collaborator

RazvanN7 commented Mar 31, 2021

Hi @a-ludi , thanks for your PR! In case this is your first PR to phobos: welcome to the Dlang community!

Generally, changes that are accepted into the standard library need to be accompanied by a strong justification.

Can you please provide a case where the current code does not work correctly? The unittest you added passes
even without your changes so it is hard to understand why this PR is needed. Note that equality is tested there
because the default predicate is a < b and alongside a > b, these are the most common predicates. This is
much faster than having to call binaryFun directly.

Copy link
Collaborator

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provide a justification for this PR.

@a-ludi
Copy link
Contributor Author

a-ludi commented Mar 31, 2021

Oh, I am sorry, that the unit test is a bad example. I did not really check it. I am going to write one that really shows the trouble.

The theoretical justification is simply that == and the predicate may yield different results which is unacceptable IMHO.

@a-ludi
Copy link
Contributor Author

a-ludi commented Mar 31, 2021

I updated the unit test accordingly. Note, that the behavior only occurs when compiling with -debug.

@RazvanN7
Copy link
Collaborator

Thanks!

@thewilsonator
Copy link
Contributor

Issue number?

@a-ludi
Copy link
Contributor Author

a-ludi commented Apr 4, 2021

I have not created an issue. Should I do so?

@RazvanN7
Copy link
Collaborator

RazvanN7 commented Apr 6, 2021

It would be great if you could create the issue and link it to this PR.

@RazvanN7
Copy link
Collaborator

RazvanN7 commented Apr 7, 2021

ping @a-ludi

@a-ludi
Copy link
Contributor Author

a-ludi commented Apr 8, 2021

Filed as issue 21810.

@thewilsonator
Copy link
Contributor

Please retitle the commit message to say "Fix issue 21810: issue description goes here"

@RazvanN7
Copy link
Collaborator

RazvanN7 commented Apr 8, 2021

Please add a commit that contains the message "Fix Issue 21810 - .." so that the bot picks it up.

@RazvanN7
Copy link
Collaborator

RazvanN7 commented Apr 8, 2021

For some reason, buildkite is always failing druntime with this PR. As far as I could tell the error seems unrelated. @thewilsonator any ideas?

@RazvanN7
Copy link
Collaborator

RazvanN7 commented Apr 8, 2021

Unfortunately, the message must contain "Fix", not "Fixed".

@a-ludi a-ludi force-pushed the check-sortedness-without-equals branch from aac3f73 to f5faee7 Compare April 8, 2021 14:25
@a-ludi
Copy link
Contributor Author

a-ludi commented Apr 8, 2021

For some reason, buildkite is always failing druntime with this PR. As far as I could tell the error seems unrelated. @thewilsonator any ideas?

Should I try rebasing the current master branch?

@a-ludi
Copy link
Contributor Author

a-ludi commented Apr 8, 2021

CircleCI failed because it could not pull docker images. Maybe a restart will fix it.

@thewilsonator
Copy link
Contributor

Should I try rebasing the current master branch?

yes

@RazvanN7
Copy link
Collaborator

yes, please rebase.

a-ludi added 4 commits April 12, 2021 09:34
Checking sortedness according to a given predicate must not use `==` operator
because it may not be in accordance with the predicate
Previous test did not trigger the undesired behavior because
`canCheckSortedness` was false because `hasAliasing!(int[]) == true`
@a-ludi a-ludi force-pushed the check-sortedness-without-equals branch from f5faee7 to b13306c Compare April 12, 2021 07:34
@a-ludi
Copy link
Contributor Author

a-ludi commented Apr 12, 2021

The rebase changed the actual error encountered in buildkite but the pipeline still fails.

@dlang-bot dlang-bot merged commit 8c42ec9 into dlang:master Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants