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

JackStouffer · 2016-09-16T08:50:36Z

No description provided.

JackStouffer · 2016-09-16T08:51:24Z

std/uni.d

@@ -8324,15 +8332,15 @@ auto asUpperCase(Range)(Range str)
    assert("hEllo".asUpperCase.equal("HELLO"));
 }

-// explicitly undocumented
+/// ditto
 auto asLowerCase(Range)(auto ref Range str)


@MartinNowak git says that you made these overloads. Why didn't you document them? I don't see why this should be hidden from the user.

It's not exactly pretty have all of those overloads in the docs. If it weren't for the fact that the secondary overloads use auto ref, I'd argue that it should all just be merged into one function that used static if, but I don't think that we should normally be putting auto ref on ranges like that. So, maybe it should be documented, but it is kind of ugly to have a bunch of overloads like that which basically only differ by their template constraints.

IMO the ddoc looks much better then the ddox version on functions with multiple overloads

JackStouffer · 2016-09-16T09:01:39Z

std/uni.d

@@ -8843,16 +8850,6 @@ void toLowerInPlace(C)(ref C[] s) @trusted pure
 {
    toCaseInPlace!(LowerTriple)(s);
 }
-// overloads for the most common cases to reduce compile time
-@safe pure /*TODO nothrow*/


This really feel like anti-pattern/hack to me, and it's why I removed it. We have a template system in order to avoid things like this

IMHO, if the attribute does not depend on the template arguments, then it should definitely be on the templated function. The only reason to rely on inference is because you have to. Otherwise, you're reducing clarity just to avoid typing.

Should I add @safe pure to the template declaration then?

When I first read your comment, I didn't realize that you were removing all of those overloads. I thought that you were just removing the attributes and that the main overload was more generic. However, it looks like the main overload is specifically for strings and that the point of the other overloads is to somehow improve compilation time by making the primary set of possible argument types explicit. My guess is that it results in toLower being instantiated only once for each of the main string types, whereas without those overloads, it would be instantiated each time it was used and that that results in faster compilation, but I don't know.

I agree that it's ugly that they're there, but I would assume that Martin wouldn't have added them if there weren't a significant gain, and I definitely don't think that they should be removed without his input.

As for the attributes, the main overload already has them. I made the mistake of assuming that what showed above the comment was the full section of code that was being removed and was looking at my local copy of the file rather than the full diff, so I made an ill-informed suggestion.

Pinging @MartinNowak

If these actually bring a noticeable decrease in compile times, which I doubt, then we have a bigger problem on our hands. The template system is specifically designed so you don't have to do things like this

JackStouffer · 2016-09-16T09:05:06Z

std/uni.d

    this(uint low, uint high)
    {
        _tuple[0] = low;
        _tuple[1] = high;
    }
+
+    /// Support for equality testing
    bool opEquals(T)(T val) const


Is this opEquals even nessesary? _tuple is the only member of this struct.

If the code is old enough, it might have needed an explicit opEquals to deal with const, but I don't think that that's the case anymore. You should probably verify that though. Another possibility is so that it can be compared with either a tuple or an array.

Another possibility is so that it can be compared with either a tuple or an array.

Yeah, that looks like what it's for

jmdavis · 2016-09-16T14:13:09Z

std/uni.d

    @property ref inout(uint) b() inout { return _tuple[1]; }
+    /// ditto
+    @property ref inout(uint[2]) tuple() inout { return _tuple; }


I don't know how much sense it makes to add this, but it's downright weird to be returning a static array and call it tuple.

I assume that the reason that you're adding this is because you can't alias _tuple to this and have _tuple be private while the alias is public? Why not just rename _tuple to tuple and leave it as public then? The alias seems to have been intended to allow mutation, which this change breaks. Regardless, if we're going to put what's being aliased in the docs, then we should probably pick a better name than tuple given that it's a static array.

I confess though that I'd be more inclined to just make it clear in the documentation that CodePointInterval can be used like a tuple and not put any any of these members in the documentation. They're not actually needed to make the usage clear, since it's pretty clearly supposed to be a named Tuple. In fact, I find it a bit odd that a and b even exist, since they're redundant with [0] and [1], though I guess that that's in line with doing something like Tuple!(uint, "a", uint, "b").

JackStouffer · 2016-09-16T14:24:25Z

@jmdavis Fixed

andralex · 2016-09-16T22:18:02Z

#4789 (comment) applies here, too

JackStouffer · 2016-09-23T08:41:39Z

I explained my methodology for marking things as public/private/package here: #4789 (comment)

andralex · 2016-10-01T02:42:32Z

Auto-merge toggled on

John-Colvin · 2016-10-13T12:19:10Z

This broke the following code (found this pull using digger bisect):

import std.uni;
import std.digest.digest;

void main()
{
    ubyte[2] a = [0, 1];
    auto b = a.toHexString.toLower;
}

test.d(7): Error: none of the overloads of 'toLower' are callable using argument types (char[4]), candidates are:
std/uni.d(8847):        std.uni.toLower(dchar c)

andralex · 2016-10-13T12:23:07Z

@JackStouffer could you please take a look

John-Colvin · 2016-10-13T12:54:29Z

Simpler version:

import std.uni;

void main()
{
    immutable(char)[2] a = "AB";
    auto b = a.toLower;
}

jmdavis · 2016-10-13T13:25:42Z

It's because the explicit overloads for string and friends were removed. I think that it's an accident that they even made that example compile, since they were meant as an optimziation, and toLower operates on ranges, whereas a static array is not a range. So, the code probably shouldn't have compiled in the first place, but it is broken now.

andralex · 2016-10-13T13:26:49Z

I'd say let's make those public and document them. Works?

JackStouffer · 2016-10-14T15:33:14Z

Ok, I completely agree that breaking this code is bad, and I intend to fix when I get the time.

BUT, I don't think we should keep this functionality around, specifically because this was obviously not the intended behavior. The purpose of those functions was to decrease compile times (which I will reiterate that that's problematic in itself), not to allow static arrays to be passed to the function. The function is clearly designed for ranges only. I propose that static array overloads be added back in, but undocumented and deprecated, to be removed one year from now. After deprecation, you can just slice your static array to get the exact same functionality.

andralex · 2016-10-14T15:34:49Z

cc @MartinNowak to weigh on that

jmdavis · 2016-10-14T21:43:38Z

Actually, if you want to take the approach of deprecating toLower for static arrays (since it was never supposed to work with them), you don't put the old, undocumented overloads back and deprecate them, because that would deprecate toLower for strings in general. Rather, you add new overloads that take static arrays by auto ref and then slice them internally to pass to the normal toLower.

I find it kind of annoying to be forced to go through the deprecation cycle because of this - particularly when it was never even the intention that static arrays be accepted - but in general, I don't think that it's a good idea to have range-based functions take static arrays. It hides the fact that they're being sliced (making it easier to screw up and have the resulting dynamic array live longer than the static array), and when we have some range-based functions accept static arrays and some not, it results in questions and confusion about the inconsistency.

Regardless, this problem shows how dicey it can be to add specific overloads for a generic function or to turn a function into a generic function. It's painfully easy to change the behavior in subtle ways.

andralex · 2016-10-14T23:20:12Z

Deprecating something that wasn't documented to then make it private sounds like belabored. Would it kill us to just document the blessed thing?

jmdavis · 2016-10-15T03:17:31Z

Deprecating something that wasn't documented to then make it private sounds like belabored. Would it kill us to just document the blessed thing?

We can. It's just that in general, range-based functions don't accept static arrays, and when they do, it seems to increase confusion (due to the inconsistency between different range-based functions and what they accept) - especially from folks who don't yet understand that static arrays aren't ranges. It's also non-obvious that the slicing is happening, which makes it easier to screw-up safety-wise (though in this case, it probably doesn't matter so much, because a new dynamic array is going to be returned anyway).

It's certainly not the end of the world if toLower/toUpper continue to accept static arrays, but they were never supposed to and only did so because the specialization Martin added accidentally worked with them. And it's annoying to get stuck accepting static arrays as a result. Then again, if those specializations really did improve compile time performance (which the comment claimed but Martin never explained), maybe we should have them, and since the only way to prevent static arrays from working with them is to templatize them (basically making them pointless), we'd just be stuck accepting static arrays in that case anyway. But it seems pretty ugly and redundant to be creating explicit overloads for most of the overloads that a templated function would generate, which is why this PR removed them in the first place.

So, ideally, toLower and toUpper wouldn't accept static arrays, but it may make more sense to just put up with the ugliness.

mihails-strasuns · 2016-11-10T17:09:31Z

This has introduced a major regression (https://issues.dlang.org/show_bug.cgi?id=16663) that has, for example, broken DCD with 2.072.0 release.

This reverts commit 1a7914a, reversing changes made to d387277. Fixes issue 16663

JackStouffer commented Sep 16, 2016

View reviewed changes

JackStouffer force-pushed the uni-private branch from 27b48c7 to d2f2e01 Compare September 16, 2016 09:00

JackStouffer commented Sep 16, 2016

View reviewed changes

JackStouffer changed the title ~~Either make private or document undocumented public symbols in std.uni~~ Make private, remove, or document undocumented public symbols in std.uni Sep 16, 2016

jmdavis reviewed Sep 16, 2016

View reviewed changes

JackStouffer added 3 commits September 16, 2016 15:23

document or make private public symbols

7bd3b2e

Document alias-this work arounds

f5b8e67

Remove undocumented template hacks

727985e

JackStouffer force-pushed the uni-private branch from d2f2e01 to 727985e Compare September 16, 2016 14:24

andralex added the pending autotester label Oct 1, 2016

andralex merged commit 1a7914a into dlang:master Oct 1, 2016

This was referenced Nov 10, 2016

Fix issue 16667: dub test fails on std.conv after upgrade to dmd 2.072.0 #4895

Closed

Fix issue 16663: std.unit.toUpper rejects an alias this to a string #4901

Merged

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

Revert "Merge pull request dlang#4790 from JackStouffer/uni-private"

c5200dd

This reverts commit 1a7914a, reversing changes made to d387277. Fixes issue 16663

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JackStouffer commented Sep 16, 2016

JackStouffer Sep 16, 2016

jmdavis Sep 16, 2016

JackStouffer Sep 16, 2016

JackStouffer Sep 16, 2016

jmdavis Sep 16, 2016

JackStouffer Sep 16, 2016

jmdavis Sep 16, 2016

JackStouffer Sep 17, 2016

JackStouffer Sep 16, 2016

jmdavis Sep 16, 2016

JackStouffer Sep 16, 2016

jmdavis Sep 16, 2016

JackStouffer commented Sep 16, 2016

andralex commented Sep 16, 2016

JackStouffer commented Sep 23, 2016

andralex commented Oct 1, 2016

John-Colvin commented Oct 13, 2016 •

edited

Loading

andralex commented Oct 13, 2016

John-Colvin commented Oct 13, 2016 •

edited

Loading

jmdavis commented Oct 13, 2016

andralex commented Oct 13, 2016

JackStouffer commented Oct 14, 2016 •

edited

Loading

andralex commented Oct 14, 2016

jmdavis commented Oct 14, 2016

andralex commented Oct 14, 2016

jmdavis commented Oct 15, 2016

mihails-strasuns commented Nov 10, 2016

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

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

Conversation

JackStouffer commented Sep 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JackStouffer commented Sep 16, 2016

andralex commented Sep 16, 2016

JackStouffer commented Sep 23, 2016

andralex commented Oct 1, 2016

John-Colvin commented Oct 13, 2016 • edited Loading

andralex commented Oct 13, 2016

John-Colvin commented Oct 13, 2016 • edited Loading

jmdavis commented Oct 13, 2016

andralex commented Oct 13, 2016

JackStouffer commented Oct 14, 2016 • edited Loading

andralex commented Oct 14, 2016

jmdavis commented Oct 14, 2016

andralex commented Oct 14, 2016

jmdavis commented Oct 15, 2016

mihails-strasuns commented Nov 10, 2016

John-Colvin commented Oct 13, 2016 •

edited

Loading

John-Colvin commented Oct 13, 2016 •

edited

Loading

JackStouffer commented Oct 14, 2016 •

edited

Loading