Skip to content

fix Issue 15027 – rangified functions no longer work with string-like types #3770

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

Merged
merged 6 commits into from
Oct 26, 2015

Conversation

MartinNowak
Copy link
Member

@MartinNowak
Copy link
Member Author

I went with fully supporting (and documenting) string-like types for any rangified functions.
That seemed more consistent than to just fix old functions for backwards compatibility.

@MartinNowak
Copy link
Member Author

@rainers @DmitryOlshansky @burner
Please review carefully, it might easily contain typos or oversights.

{
enum isStringLike = is(StringTypeOf!T) && (isAggregateType!T || isStaticArray!T);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Examples?

@rainers
Copy link
Member

rainers commented Oct 24, 2015

Wow, what a feat.

Looks good to me for most of it, but some constraints are difficult to verify. Can you elaborate why adding isNarrowString was necessary sometimes?

I also just noticed that rangification also changed behaviour for structs that satisfy range constraints, but also alias this to a string type. These are restored with the isStringLike check, but is this actually desired?

@@ -58,6 +58,22 @@ module std.path;
import std.file; //: getcwd;
import std.range.primitives;
import std.traits;
import std.meta : anySatisfy;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are selective imports fixed or does this still add a public alias? If the latter, using static import might be better if you want to avoid pollution of symbol lookup in this module.

@MartinNowak
Copy link
Member Author

Can you elaborate why adding isNarrowString was necessary sometimes?

I tried to consolidate all the different constraint variations, and dstring doesn't need to be handled separately b/c it already offers random access et.al. It also seems that isSomeString is slightly more expensive for the compiler to compute than isNarrowString.

@MartinNowak
Copy link
Member Author

I also just noticed that rangification also changed behaviour for structs that satisfy range constraints, but also alias this to a string type. These are restored with the isStringLike check, but is this actually desired?

This was done on purpose b/c isInputRange is true for structs w/ alias this string, and we don't want the possibly expensive copy in that case.
And I'd call a struct that offers a stringish range interface and still aliases to a string illformed b/c it effectively offers two range interfaces, any practically relevant example?

@rainers
Copy link
Member

rainers commented Oct 25, 2015

I thought it was only theoretically, but just grepped phobos and e.g. takeExactly returns a range that aliases this to another range. Not sure if this could trigger the change, though.

- select only structs/static array that are implicitly convertible to a string

- also add internal peelStringLike helper
@MartinNowak
Copy link
Member Author

takeExactly returns a range that aliases this to another range.

It aliases to Take!N which is just a variant of takeExactly.

@MartinNowak
Copy link
Member Author

It doesn't work for writeln as well, so I don't think anyone is relying on that for strings.
http://dpaste.dzfl.pl/15e976628ebe

@MartinNowak MartinNowak force-pushed the string_like branch 3 times, most recently from 6b9fc33 to bfc33e7 Compare October 25, 2015 19:13
@MartinNowak
Copy link
Member Author

Passes now, and nothing left to do.

@MartinNowak
Copy link
Member Author

Anything left to do? I want to build a release candidate.

@DmitryOlshansky
Copy link
Member

I think I've read it a few times. Nothing strikes me as too odd.

@DmitryOlshansky
Copy link
Member

Auto-merge toggled on

DmitryOlshansky added a commit that referenced this pull request Oct 26, 2015
fix Issue 15027 – rangified functions no longer work with string-like types
@DmitryOlshansky DmitryOlshansky merged commit 384410a into dlang:stable Oct 26, 2015
@andralex
Copy link
Member

I have written a note to phobos@puremagic.com which I think is important enough to paste here as well:

Hi folks, isStringLike is a fine idea but there are a number of issues I noticed while looking at the PR and the related pieces it works with.

  1. isAggregate is not the best choice of names for something that's a struct, class, union, or interface. In C++ aggregates are a completely different notion (http://stackoverflow.com/questions/4178175/what-are-aggregates-and-pods-and-how-why-are-they-special) so essentially we're defining our own terminology. Not the best thing to do. Closest term I can think of is isUserDefinedType, but that would include enumerated types.

The larger question is why this peculiar notion is needed strongly enough to get its own name.

  1. I take issue with the current definition of isStringLike, which is:

    (isAggregateType!T || isStaticArray!T) && is(StringTypeOf!T);

So... a string is unlike a string, which I find unfitting. Furthermore, the entire notion of isStringLike and StringTypeOf is dedicated to work around what seems to be a bug in the typesystem (alias this fails to implement real subtyping). It seems that is the right place to focus work on.

A worse problem is that isStringLike fails to capture the more frequent "string-like" structure: (a) lazy ranges of characters and (b) reference counted strings. These are crucial pieces. So we're not making progress toward actually defining what we think a string-like data structure is.

I think a string-like structure is:

(a) an array of characters or a subtype thereof

(b) some other forward range with ElementType some character type, and potentially ElementEncodingType a different character type as well.

That's pretty much it. Algorithms dealing with string-like data should be able to work with this definition, or temporarily convert to arrays of characters.

More importantly, while looking at #3770 and related work I can't stop noticing a growth of layers of complexity that seems poorly scalable, instead of an increase in generic simplicity and clarity of concepts. That does not bode well and we need to curb it lest we end up with a standard library we ourselves can't make sense of.

I do understand there are issues to fix and we need to move forward. But tactical solutions cannot substitute a lack of strategy.

Please let us all stop and draw a little breath, and figure out how to replace the monumental work in PR 3770 with simplifying abstractions. Once something like isStringLike becomes part of Phobos, we're stuck with it for eternity - there's no way we can fix it without breaking code.

I don't have the bandwidth to scrutinize all Phobos changes and at the same time work on refcounted safe collections (my focus). But please keep me informed of all public name additions to Phobos, and also discuss them in the forum. If we keep on introducing new notions at this pace, we'll drown Phobos in its own attempts at being rigorous.

@andralex
Copy link
Member

To clarify: I'm opposed to deploying a release with this solution.

@MartinNowak
Copy link
Member Author

The name and concept string-like are not the best fit but we need a workaround for type deduction deficiencies (and fixing that is completely out of scope for the overdue 2.069.0).

More importantly, while looking at #3770 and related work I can't stop noticing a growth of layers of complexity that seems poorly scalable, instead of an increase in generic simplicity and clarity of concepts.

I think everybody agrees that this is an ugly workaround and we want something like foo(R : isInputRange)(R range) to become part of the type deduction. Meanwhile we cannot break most of std.{file,path,string} with the next release.

Maybe the solution is as simple as leaving isStringLike as internal helper and undocumenting the overloads?

@MartinNowak MartinNowak deleted the string_like branch October 26, 2015 17:01
@MartinNowak
Copy link
Member Author

I do understand there are issues to fix and we need to move forward. But tactical solutions cannot substitute a lack of strategy.

Unfortunately this problem was ignored for several month and is now blocking the release.

@andralex
Copy link
Member

Maybe the solution is as simple as leaving isStringLike as internal helper and undocumenting the overloads?

Yes, that seems a prudent path to follow for the time being. Thanks!

@MartinNowak
Copy link
Member Author

see #3774

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.

5 participants