Skip to content

Comments

Optimize count with an overload which calls walkLength#5557

Merged
dlang-bot merged 1 commit intodlang:masterfrom
RazvanN7:Optimize_count
Jul 6, 2017
Merged

Optimize count with an overload which calls walkLength#5557
dlang-bot merged 1 commit intodlang:masterfrom
RazvanN7:Optimize_count

Conversation

@RazvanN7
Copy link
Collaborator

@RazvanN7 RazvanN7 commented Jul 6, 2017

Following the discussion in #5545 , I created this separate PR for the count optimization. The implementation uses walkLength to compute the count. If walkLength will be deprecated, then I will replace the call with the code, otherwise we can close the other PR and keep just this oprimization

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

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.

@UplinkCoder
Copy link
Member

Why is that needed.

The ddoc is bigger then the function ?
which is a thunk.
a templated thunk no less.
introducing symbol bloat.
You could just have made an alias you know ...

@RazvanN7
Copy link
Collaborator Author

RazvanN7 commented Jul 6, 2017

@UplinkCoder That was the consensus reached in #5545. I guess creating an alias might be better, still the documentation needs to be there

@wilzbach
Copy link
Contributor

wilzbach commented Jul 6, 2017

You could just have made an alias you know ...

Would break code, due to the different default behavior. count allows a user-defined pred, walkLength allows a maximal count.
Though imho on the long run on of these functions should be deprecated (or at least removed from the docs).

The ddoc is bigger then the function ?

Because documentation is a lot more important than code. This is a common pattern in Phobos ;-)

@RazvanN7
Copy link
Collaborator Author

RazvanN7 commented Jul 6, 2017

@wilzbach I created an overload anyway so making an alias won't break any code, but it seems cleaner to me to have an overload, rather then an alias.

Copy link
Contributor

@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.

LGTM to me in general. If we decide to deprecate on of the symbols we can always do this afterwards as this is just an optimization.

@wilzbach
Copy link
Contributor

wilzbach commented Jul 6, 2017

(keeping this open for one or two days to give people enough time to veto this approach as a general good practice)


The fourth version counts the number of elements in a range. It is
an optimization for the third version: if the given range has the
length property the count is returned right away, otherwise
Copy link
Member

Choose a reason for hiding this comment

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

length in code font (e.g. backticks)

@andralex
Copy link
Member

andralex commented Jul 6, 2017

Aliases are generally better, but here we have overloading so that wouldn't work well.

This improves the situation whereby someone calls count with the default predicate - avoid the goofy walking the whole range. That's fine, as is to allow two distinct library functions sometimes do the same thing under certain circumstances.

@RazvanN7
Copy link
Collaborator Author

RazvanN7 commented Jul 6, 2017

@andralex done

@dlang-bot dlang-bot merged commit 63f6426 into dlang:master Jul 6, 2017
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.

5 participants