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

Add algorithms: filter, map and equals #11643

Merged
merged 1 commit into from
Sep 5, 2020

Conversation

jacob-carlborg
Copy link
Contributor

Also adds function to create static arrays.

Supersedes #11586.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @jacob-carlborg! 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

Your 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 locally

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

dub run digger -- build "master + dmd#11643"

src/dmd/root/array.d Outdated Show resolved Hide resolved
src/dmd/root/array.d Outdated Show resolved Hide resolved
src/dmd/root/array.d Show resolved Hide resolved
src/dmd/root/array.d Outdated Show resolved Hide resolved
src/dmd/root/array.d Show resolved Hide resolved
}

alias opDollar = length;
}
Copy link
Member

Choose a reason for hiding this comment

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

I would add opSlice overloads, s.t. the ranges can be saved/copied (I see that you have already used this for built-in arrays)

Copy link
Contributor Author

@jacob-carlborg jacob-carlborg Aug 29, 2020

Choose a reason for hiding this comment

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

That looks like it's going to be a bit more complicated. As in quite a bit of extra code need to be added. I had to draw the line somewhere to minimize the code that needed to be added.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, we can always add it when needed.

src/dmd/root/array.d Show resolved Hide resolved
@jacob-carlborg jacob-carlborg force-pushed the algorithms branch 2 times, most recently from 0ebe3d5 to 295ba68 Compare August 29, 2020 18:41
@jacob-carlborg
Copy link
Contributor Author

Win32 on the auto-tester times out every time. The failing test in buildkite for pbackus/sumtype seems unrelated.

@wilzbach
Copy link
Member

Buildkite should be unbroken by dlang/phobos#7611. I don't know what's the story with win32.

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Great start 👍

src/dmd/root/array.d Outdated Show resolved Hide resolved
src/dmd/root/array.d Outdated Show resolved Hide resolved
@wilzbach wilzbach added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Sep 3, 2020
@wilzbach
Copy link
Member

wilzbach commented Sep 3, 2020

I think this is a great start. Any other objections / opinions?

Also adds function to create static arrays.
@ghost ghost removed the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Sep 3, 2020
@ghost
Copy link

ghost commented Sep 3, 2020

Yes sorry I object. This PR needs a second commit that does refactoring using the new functions. You see just at a few places.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

prove it is required in a second commit

@ghost
Copy link

ghost commented Sep 3, 2020

well "required" is not what I mean. what i mean is more like "these abstractions make the code more pleasant to read and easy to understand".

@jacob-carlborg
Copy link
Contributor Author

@wilzbach
Copy link
Member

wilzbach commented Sep 4, 2020

prove it is required in a second commit

We're talking about the "heart of Phobos" here. There's really no need to show why lazy, non-allocating filter and map are useful for writing maintainable code. We have been talking on and off about introducing it to DMD for years.
If you need more convincing you can just open any D project on the dub registry and I bet 95% you'll find at least one of the functions used ;-)

@ghost
Copy link

ghost commented Sep 4, 2020

This is not good enough https://github.com/jacob-carlborg/dmd/blob/f9d8476176adc0a04a54b74c1aad0292356fe4df/src/dmd/objc_glue.d#L1392?

Why not start using it directly in dmd ? as far as i understand the example is in the fork modified for visual-d.

@jacob-carlborg
Copy link
Contributor Author

Why not start using it directly in dmd

Because as a good citizen I split up my changes into multiple PRs to make them more manageable. 9 of my last 10 merged PRs have been all refactorings and preparations for adding support for Objective-C protocols. This one is that as well. If you insist, I know of an existing place that could benefit, but that would require walkLength as well.

as far as i understand the example is in the fork modified for visual-d

No, it definitely is not. I don't work at or use visual-d at all. If you haven't noticed, I use Mac 😉.

@ghost
Copy link

ghost commented Sep 4, 2020

Indeed and sorry. Rainers S does, not you.

@jacob-carlborg
Copy link
Contributor Author

Indeed and sorry. Rainers S does, not you.

No worries 😃.

@dlang-bot dlang-bot merged commit d484e6c into dlang:master Sep 5, 2020
@jacob-carlborg
Copy link
Contributor Author

Thanks.

@jacob-carlborg jacob-carlborg deleted the algorithms branch September 5, 2020 06:13
@nordlow
Copy link
Contributor

nordlow commented Sep 5, 2020

BTW: for more array only overloads (non-template bloated) of Phobos algorithms see my nxt.array_algorithm here and here.

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