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

fix Issue 13507 - std.range.enumerate with BigInt indexes too #6496

Conversation

Voultapher
Copy link

Seems to work with modified existing unit test. Didn't find a good way to test enumerate range with a BigInt worthy size.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @Voultapher! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
  • 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
13507 enhancement std.range.enumerate with BigInt indexes too

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + phobos#6496"

@Voultapher Voultapher force-pushed the feature/enumerate-with-bigint-index branch from f87dd96 to 02e3f36 Compare May 5, 2018 14:10
@Voultapher Voultapher force-pushed the feature/enumerate-with-bigint-index branch from 02e3f36 to e6c5848 Compare May 5, 2018 14:19
@@ -10114,6 +10115,26 @@ pure @safe unittest
}}
}

/// enumerate works with BigInt
@system pure unittest
Copy link
Member

Choose a reason for hiding this comment

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

With three /// this will be part of the documentation (hence the CI failure).
I recommend doing the following:

  • have a small public, minimal test
  • use a more complex private test or tests

static assert(is(typeof(enumerated.front.index) == BigInt));
assert(offsetEnumerated.equal(subset.zip(subset)));
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a test with very big numbers?

{
auto subset = values[cast(size_t) i .. $];
auto offsetEnumerated = subset.enumerate!BigInt(i);
static assert(is(typeof(enumerated.front.index) == BigInt));
Copy link
Member

Choose a reason for hiding this comment

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

This is not influenced by the loop (and already tested before)
Maybe you meant offsetEnumerated?


auto enumerated = values.enumerate!BigInt();
static assert(is(typeof(enumerated.front.index) == BigInt));
assert(enumerated.equal(values[].zip(values)));
Copy link
Member

Choose a reason for hiding this comment

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

no need for the slicing here.

@Voultapher
Copy link
Author

As discussed with @wilzbach, the feature could be implemented, in a minimal working way. Yet, trying to test it revealed ergonomic issues with other library components. For example, getting an enumerate range to a point where BigInt is necessary, should be possible with:

const auto upperBound = BigInt("1_000_000_000_000_000_000_000");
auto range = iota(upperBound).enumerate!BigInt;
range.popFrontN(upperBound - 10);

For this to work, iota!BigInt would have to return a range than has a length and can be sliced. However hasLength is defined and constrained to a length function that returns size_t, and this is only one of the problems faced. Short of changing vast parts of phobos or duplicating a lot of code to support BigInt, I don't see a way to implement this ergonomically.

@MetaLang
Copy link
Member

MetaLang commented May 12, 2018

However hasLength is defined and constrained to a length function that returns size_t, and this is only one of the problems faced.

I've got an idea for how to fix hasLength, but what are the other problems? If you can list them we can go through one by one and come up with a solution.

@jmdavis
Copy link
Member

jmdavis commented May 12, 2018

I've got an idea for how to fix hasLength, but what are the other problems? If you can list them we can go through one by one and come up with a solution.

If you want to change hasLength, you're going to need @andralex's approval. After repeated discussions on the topic (and after reverting changes in some cases), we've stuck with size_t for length, because allowing anything else for length complicates life considerably. The fact that length is explicitly size_t and not anything else is very much on purpose.

@Voultapher
Copy link
Author

This was tagged as a Bootcamp issue, now it has evolved into a wider design discussion. I'd be glad if someone more experienced with the codebase could take over.

@Voultapher
Copy link
Author

This has not seen any progress in years so I'm closing it.

@Voultapher Voultapher closed this Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants