Navigation Menu

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 missing void back(bool) in Array!bool.Range #1656

Merged
1 commit merged into from Oct 25, 2013
Merged

Fix missing void back(bool) in Array!bool.Range #1656

1 commit merged into from Oct 25, 2013

Conversation

monarchdodra
Copy link
Collaborator

Forked from #1010

@ghost
Copy link

ghost commented Oct 23, 2013

Is there a test-case already in place for this? I imagine not. :)

@monarchdodra
Copy link
Collaborator Author

Is there a test-case already in place for this? I imagine not. :)

Nope. I'll write the minimal test that covers all functions.

@monarchdodra
Copy link
Collaborator Author

Ended up adding some missing opDollar, a missing opSlice, and changing some size_t to ulong. Although arguably, I think I should have instead changed to size_t?

@ghost
Copy link

ghost commented Oct 24, 2013

Why did it use ulong to begin with? I think we should use size_t.

@monarchdodra
Copy link
Collaborator Author

Why did it use ulong to begin with? I think we should use size_t.

The idea is that someone may want to allocate 8_000_000_000 bools on a 32 bit system, which is perfectly fine (takes 1G memory), but it requires a type larger than size_t to access all those bools.

That said, I don't think this special ability warrants the problems that come with not using size_t. C++'s vector<bool> also uses size_t: http://www.cplusplus.com/reference/vector/vector-bool/

I'll change everything back to size_t then. Seems smarter...

@ghost
Copy link

ghost commented Oct 24, 2013

It seems like the job of Array!bool is to compact memory, but not necessarily allow a greater number of elements. So using size_t is appropriate IMO.

@jmdavis
Copy link
Member

jmdavis commented Oct 25, 2013

Any attempts to use anything other than size_t for length should be viewed with extreme prejudice. I don't know that we should never do it, but there had better be a very good reason for doing it, because dealing with it in generic code is going to be horrible. I think that it was a huge mistake to make it so that iota uses anything other than size_t for length, and I don't want to see the same mistake elsewhere (in fact, I'd love to see it removed in iota). If someone really needs that many elements on a 32-bit system, they should probably just roll their own solution. The cost to supporting it in the standard library is too high IMHO.

Forked from #1010

Also some cleanup:
changed ulong to size_t, some spaces, some Tuples, some "!(T)"=>"!T"
@monarchdodra
Copy link
Collaborator Author

Done. I also filed an issue for it:
http://d.puremagic.com/issues/show_bug.cgi?id=11349
Since I suppose it would warrant an entry in the change log.

Also, removed some gratuitous (IMO) Tuple usage, when a simple struct would have done.

I think this is good for review now.

@ghost
Copy link

ghost commented Oct 25, 2013

LGTM.

@ghost ghost closed this Oct 25, 2013
@ghost ghost reopened this Oct 25, 2013
ghost pushed a commit that referenced this pull request Oct 25, 2013
Fix missing `void back(bool)` in Array!bool.Range
@ghost ghost merged commit 9d48f6f into dlang:master Oct 25, 2013
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants