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

Trivial Improvements To SortedRange #3669

Merged
merged 1 commit into from
Sep 23, 2015
Merged

Conversation

JackStouffer
Copy link
Member

  • removed dead code
  • moved imports to local

@JackStouffer
Copy link
Member Author

I just tested this and this change fixes Issue 15003 https://issues.dlang.org/show_bug.cgi?id=15003

@schveiguy
Copy link
Member

OK, so 2 things:

  1. Change commit message to say "fix issue 15003" so bugzilla will close it automatically
  2. Add unit test to verify 15003 is fixed.

Note: I'm surprised the text(st) doesn't allocate. I would think it would.

@JackStouffer
Copy link
Member Author

Note: I'm surprised the text(st) doesn't allocate. I would think it would.

That's because it does. I was running the unit tests for the wrong package. I wonder how many times I have to make that mistake before I stop.

However, I believe that my new change to remove the text call here is justified, as just printing out the tested subset of the passed range to the user doesn't make much sense to begin with. Honestly, the question of checking sorted-ness at all in a function called assumeSorted seems weird to me, but that's a discussion for a future PR.

@schveiguy
Copy link
Member

Honestly, the question of checking sorted-ness at all in a function called assumeSorted seems weird to me

It's kind of a strange thing, but I think it's correct. When you are saying assumeSorted, you are making a blind assumption that the type system does not check. In debug mode, it's useful to have the compiler double-check your work, to catch any specific bugs.

The removal of the text function is unfortunate, since it provides a clear idea to the user why the range isn't sorted. I'm wondering whether it would be possible for the compiler to ignore gc-allocating functions inside an assert for @nogc purposes, or possibly with assert while inside debug code. But that's a separate discussion.

@schveiguy
Copy link
Member

LGTM

@yebblies
Copy link
Member

I'm wondering whether it would be possible for the compiler to ignore gc-allocating functions inside an assert for @nogc purposes, or possibly with assert while inside debug code. But that's a separate discussion.

It really should be doing that, like it ignores pure violations in debug blocks.

@JackStouffer
Copy link
Member Author

It really should be doing that, like it ignores pure violations in debug blocks.

https://issues.dlang.org/show_bug.cgi?id=15100

@burner
Copy link
Member

burner commented Sep 23, 2015

LGTM

@burner
Copy link
Member

burner commented Sep 23, 2015

Auto-merge toggled on

burner added a commit that referenced this pull request Sep 23, 2015
Trivial Improvements To SortedRange
@burner burner merged commit e968ff3 into dlang:master Sep 23, 2015
@JackStouffer JackStouffer mentioned this pull request Sep 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants