[trivial] Either make private or document undocumented public symbols in stdio by JackStouffer · Pull Request #4789 · dlang/phobos

JackStouffer · 2016-09-15T09:12:42Z

No description provided.

9il · 2016-09-15T14:02:47Z

What the reason of that change? This will break existing code

jmdavis · 2016-09-15T14:44:20Z

std/stdio.d

 be compatible with the access attributes of the handle. Windows only.

-Throws: $(D ErrnoException) in case of error.
+Throws: $(D ErrgnoException) in case of error.


Somehow, I don't think that you meant to insert a g here.

jmdavis · 2016-09-15T14:46:17Z

std/stdio.d

    }
-    // no UTF checking
+    /// Implements `opApply` `foreach` support but with no UTF checking
    int opApplyRaw(D)(scope D dg)


This looks like it's just intended to be used by opApply and should not be treated as public.

ok, will mark as private

jmdavis · 2016-09-15T14:47:55Z

What the reason of that change? This will break existing code

If something isn't documented, then it doesn't exist. Anyone who uses an undocumented symbol has no guarantees that their code won't be broken by changes to it later.

9il · 2016-09-15T15:09:44Z

If something isn't documented, then it doesn't exist. Anyone who uses an undocumented symbol has no guarantees that their code won't be broken by changes to it later.

I don't agree. No guarantees, but it still exists. And an engineer do not need to spend time by copypasting code

JackStouffer · 2016-09-15T15:11:59Z

These symbols were completely undocumented. The only way people were using these is if they found them by pure chance or they were looking at Phobos code. Either way, they aren't officially part of the Phobos API, so this is not a breaking change

codecov-io · 2016-09-15T15:13:57Z

Current coverage is 88.86% (diff: 100%)

Merging #4789 into master will decrease coverage by <.01%

@@             master      #4789   diff @@
==========================================
  Files           121        121          
  Lines         74221      74221          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          65959      65958     -1   
- Misses         8262       8263     +1   
  Partials          0          0

Powered by Codecov. Last update 3142577...121af02

JackStouffer · 2016-09-15T15:15:15Z

FIxed

9il · 2016-09-15T15:15:32Z

The only real way to understand phobos is to look into its code. Our documentation is not so good

jmdavis · 2016-09-15T15:18:30Z

The only real way to understand phobos is looking into its code. Out documentation is not so good

It's as good or better than most of the documentation for C++'s standard library, and you don't go looking though the STL and decide to use undocumented symbols do you?

9il · 2016-09-15T15:34:15Z

The only real way to understand phobos is looking into its code. Out documentation is not so good
It's as good or better than most of the documentation for C++'s standard library, and you don't go looking though the STL and decide to use undocumented symbols do you?

I don't. STL has much more users and online activity. STL and Boost sources looks terrible for me. Phobos sources are readable and clean.

jmdavis · 2016-09-15T16:12:52Z

I don't. STL has much more users and online activity. STL and Boost sources looks terrible for me. Phobos sources are readable and clean.

So, because there are fewer people to ask questions about Phobos, and it's actually readable, you think that folks should be able to expect to rely on undocumented symbols? I really don't think that that's reasonable. If they were intended to be part of the public API, they would be documented. The fact that they're public is either a mistake or a workaround for a problem and not because it's something that anyone should be using.

jmdavis · 2016-09-15T16:13:36Z

@JackStouffer It looks like you have some trailing whitespace, and the auto-tester doesn't like that.

WalterBright · 2016-09-16T06:04:32Z

I think that code was written by @andralex I think he should comment on whether they are meant to be public or not.

WalterBright · 2016-09-16T06:06:55Z

@JackStouffer Please in the future use more descriptive branch names than things like package2, which would be helpful when looking at https://auto-tester.puremagic.com/pulls.ghtml?projectid=1

This could be called stdio-private, for example. Something to jog the memory on what it is about.

andralex · 2016-09-16T22:09:17Z

There are two kinds of undocumented public symbols: (a) some that were inadvertent, (b) some that used to be documented and have been shunned by being left out there but undocumented in order to not break code.

I encourage making those in category (a) private, but (b)s should be left alone (at best with a comment indicating they're to stay put. @JackStouffer could you do some detective work on the file history to see which is which? Thx!

JackStouffer · 2016-09-19T09:32:29Z

Here's my philosophy:

If something is public, then it's meant to be used by the programmer, unless it's specifically set for deprecation
If something is not meant to be used by the programmer, it should be private (again, baring deprecation)
If it's meant to be used by the programmer, then the programmer should know about it, i.e. it should be documented

I believe these changes fall in line with that. The structs were never meant to be used directly; you're supposed to use the factory functions.

schuetzm · 2016-09-19T10:00:40Z

I expect category (b) to have a (non-ddoc) comment stating that the symbols are there for legacy reasons.

andralex · 2016-09-23T19:41:00Z

Auto-merge toggled on

ghost · 2016-11-11T21:35:54Z

breakage confirmed since 2.072. DFMT uses a few of these symbols. See https://github.com/Hackerpilot/dfmt/blob/master/src/dfmt/main.d#L118

qchikara · 2016-11-12T22:08:16Z

I suggest that these should be deprecated BEFORE private.
If we can make it private, we could also rename to _privateName and add a temporary alias orgName with the attr deprecated, then eventually omit such alias.

mihails-strasuns · 2016-11-15T10:29:33Z

@qchikara that is indeed a correct (and required) approach.

This reverts commit c940f5a, reversing changes made to aaf31b5. Fixes issue 16682

9il added the Review:Needs Decision label Sep 15, 2016

jmdavis reviewed Sep 15, 2016

View reviewed changes

jmdavis added Review:Needs Work and removed Review:Needs Decision labels Sep 15, 2016

JackStouffer force-pushed the package2 branch from a6a6b82 to aa94232 Compare September 15, 2016 15:14

stdio: either make private or document undocumented public symbols

8ef278d

JackStouffer force-pushed the package2 branch from aa94232 to 8ef278d Compare September 15, 2016 18:02

andralex mentioned this pull request Sep 16, 2016

Make private, remove, or document undocumented public symbols in std.uni #4790

Merged

andralex merged commit c940f5a into dlang:master Sep 23, 2016

ghost mentioned this pull request Nov 11, 2016

Impossible to build under Windows with phobos 2.072 dlang-community/dfmt#263

Closed

John-Colvin pushed a commit to John-Colvin/phobos that referenced this pull request Dec 8, 2016

Revert "Merge pull request dlang#4789 from JackStouffer/package2"

a6c2e11

This reverts commit c940f5a, reversing changes made to aaf31b5. Fixes issue 16682

JackStouffer deleted the package2 branch May 22, 2017 17:29

Uh oh!

Comments

Conversation

JackStouffer commented Sep 15, 2016

Uh oh!

9il commented Sep 15, 2016

Uh oh!

jmdavis Sep 15, 2016

Choose a reason for hiding this comment

Uh oh!

JackStouffer Sep 15, 2016

Choose a reason for hiding this comment

Uh oh!

jmdavis Sep 15, 2016

Choose a reason for hiding this comment

Uh oh!

JackStouffer Sep 15, 2016

Choose a reason for hiding this comment

Uh oh!

jmdavis commented Sep 15, 2016

Uh oh!

9il commented Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JackStouffer commented Sep 15, 2016

Uh oh!

codecov-io commented Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 88.86% (diff: 100%)

Uh oh!

JackStouffer commented Sep 15, 2016

Uh oh!

9il commented Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmdavis commented Sep 15, 2016

Uh oh!

9il commented Sep 15, 2016

Uh oh!

jmdavis commented Sep 15, 2016

Uh oh!

jmdavis commented Sep 15, 2016

Uh oh!

WalterBright commented Sep 16, 2016

Uh oh!

WalterBright commented Sep 16, 2016

Uh oh!

andralex commented Sep 16, 2016

Uh oh!

JackStouffer commented Sep 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schuetzm commented Sep 19, 2016

Uh oh!

andralex commented Sep 23, 2016

Uh oh!

ghost commented Nov 11, 2016

Uh oh!

qchikara commented Nov 12, 2016

Uh oh!

mihails-strasuns commented Nov 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

9il commented Sep 15, 2016 •

edited

Loading

codecov-io commented Sep 15, 2016 •

edited

Loading

9il commented Sep 15, 2016 •

edited

Loading

JackStouffer commented Sep 19, 2016 •

edited

Loading