Skip to content

Comments

fix issue 15364 - BitArray.len should be private#3822

Merged
schveiguy merged 1 commit intomasterfrom
unknown repository
Nov 19, 2015
Merged

fix issue 15364 - BitArray.len should be private#3822
schveiguy merged 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Nov 19, 2015

No description provided.

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
15364 BitArray.len should be private

@JakobOvrum
Copy link
Contributor

LGTM

1 similar comment
@JackStouffer
Copy link
Contributor

LGTM

@schveiguy
Copy link
Member

These public members were all not documented, so it makes sense that we don't need to worry about breakage (e.g. bitsPerSizeT was harmlessly public)

@schveiguy
Copy link
Member

whitelisted @Blumerline

@schveiguy
Copy link
Member

Auto-merge toggled on

@JackStouffer
Copy link
Contributor

whitelisted @Blumerline

Whitelisted for what?

schveiguy added a commit that referenced this pull request Nov 19, 2015
fix issue 15364 - BitArray.len should be private
@schveiguy schveiguy merged commit c254fec into dlang:master Nov 19, 2015
@schveiguy
Copy link
Member

Whitelisted for what?

autotester.

@ghost ghost mentioned this pull request Jan 11, 2016
@WalterBright
Copy link
Member

Yeah, except they did break things :-(

@schveiguy
Copy link
Member

Any broken code was using undocumented features that were not meant to be accessible. If read-only access is necessary, then we can add public accessors. But ptr and len should not be writable.

MartinNowak added a commit to MartinNowak/phobos that referenced this pull request Jan 24, 2016
- keep ptr and len as deprecated (unsafe) properties
@MartinNowak
Copy link
Member

We always need to worry about breakage, this was so unnecessary.
Nobody would have an issue w/ properly deprecating access to those fields, but just randomly breaking code b/c sth. should have been done different years ago is unacceptable.
#3950

jmdavis added a commit that referenced this pull request Jan 25, 2016
@schveiguy
Copy link
Member

No, changing len or ptr or both externally is neither supported nor could possibly be accepted as valid code.

Any code that broke because of this had a bug, and now the person would see the bug should be fixed. We did anyone who's code broke a favor.

This is like saying fixing a buffer overrun broke my code.

@Hackerpilot
Copy link
Contributor

@MartinNowak
Copy link
Member

Any code that broke because of this had a bug, and now the person would see the bug should be fixed. We did anyone who's code broke a favor.

It certainly wasn't used incorrectly in undead, and every breakage we introduce requires library maintainers to release new versions and uses to update their dependencies.
If we don't deprecate such changes properly we cause a lot of downstream work and devaluate existing D code. We should be more careful with our tiny ecosystem.

@schveiguy
Copy link
Member

It certainly wasn't used incorrectly in undead

Certainly it wasn't when it was part of Phobos. As a separate 3rd party library (and isn't this the entire purpose of this library, to maintain support with the current phobos?), it should not be accessing internals of Phobos implementation. So yes, it was used incorrectly (although now fixed).

We can agree to disagree on this. I don't think we should cater or support any 3rd party library that uses internal details of Phobos implementations. You can disagree with that, but I can't have any care or sympathy for those libraries, sorry.

@MartinNowak
Copy link
Member

We don't disagree on the fact that internals should be internal, but I'm arguing against simply breaking things b/c someone made a mistake years ago. The cost of properly deprecating those fields is literally zero.

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.

7 participants