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

Removed string predicates from std.algorithm.iteration documentation #3800

Merged
merged 1 commit into from
Jan 16, 2016

Conversation

JackStouffer
Copy link
Member

The two most common things that I hear when introducing D to people are

  • "Is writeln actually a property of string!?" (misunderstanding of UFCS)
  • "Does D really use strings instead of lambdas?"

Nothing much can be done for the first one, idiosyncrasies are part of learning a new language. But the second one is mostly because the docs use string predicates more than lambdas. String predicates are bad practice compared to lambdas IMO and should be discouraged by not including them in docs. Yes, there are legitimate uses of string predicates, but 99% of the time, you should use lambdas.

@schveiguy
Copy link
Member

I don't agree. Strings are perfectly acceptable here, and offer another benefit (besides brevity) that some template with a string parameter is a singleton, while templates based on lambdas are unique.

For instance, the following code creates 2 instances of template filter:

auto foo(R)(R r) if(isInputRange!R)
{
   return filter!(a => a==5)(r);
}

auto bar(R)(R r, int x) if(isInputRange!R)
{
   return sum(r.filter!(a => a==5), x)
}

while using string lambdas only produces one. Saves on code bloat, and other things. Until we can factor those into one instance, I think string lambdas are here to stay.

What can be done, however, is to mix strings and lambdas in the docs so it's obvious both are possible.

However, I'd say in the cheat sheet, go ahead and change everything to lambdas, it should be consistent (right now, it looks like some functions only take strings, while others take lambdas).

@JackStouffer
Copy link
Member Author

IMO exe size is a small price to pay for syntax highlighting, better errors from the compiler, the ability to forget about special cases like the fact that "a++" modifies in place, and the ability to name parameters whatever you want.

Also, my point still stands that changing these makes the docs clearer for new comers. From these examples you can infer you can pass function literals or function pointers logically from the fact that you can pass lambdas. Where as the string examples, you can be forgiven for believing that you can only use strings. I mean, we can't expect every newcomer to read Ali's or Andrei's book for the full picture; a lot of people are just going to be reading the Phobos docs.

@schveiguy
Copy link
Member

IMO exe size is a small price to pay for syntax highlighting, better errors from the compiler, the ability to forget about special cases like the fact that "a++" modifies in place, and the ability to name parameters whatever you want.

It's a small price, but it's a price. I'm just saying that we shouldn't remove string lambdas because they offer benefits (for now), and showing that both strings and lambdas work in the examples identifies the capabilities phobos supports. I don't want to remove all string lambdas from the docs or from phobos.

@JackStouffer
Copy link
Member Author

I'm just saying that we shouldn't remove string lambdas because they offer benefits (for now), and showing that both strings and lambdas work in the examples identifies the capabilities phobos supports.

The problem is that it doesn't make much sense to have two lines for every example with a predicate defined, one with a string and one with a lambda.

How about this: because of the problem above, we keep lambdas in the docs, as I think that they are the best option most of the time. But, we modify the docs of these functions so fact that the predicate is passed to std.functional.binaryFun is made clear with a link directly to binaryFun docs so people know what their options are, including strings.

@JackStouffer
Copy link
Member Author

@schveiguy ping.

@schveiguy
Copy link
Member

Can you modify one doc and show me what you mean?

@JackStouffer JackStouffer force-pushed the lambda branch 2 times, most recently from 346e56f to 4f6226c Compare November 25, 2015 15:09
@JackStouffer
Copy link
Member Author

@schveiguy I modified the docs for splitter as an example.

@@ -2864,6 +2864,9 @@ Two adjacent separators are considered to surround an empty element in
the split range. Use $(D filter!(a => !a.empty)) on the result to compress
empty elements.

The predicate is passed to $(XREF functional,binaryFun), and can accept a string,
function pointer, lambda, or function literal.

Copy link
Member

Choose a reason for hiding this comment

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

There are many things that work here, including delegates, functors, functions, templates, etc.

I think it's probably best to identify that strings here are a special case. Everything else is an alias to a "callable" entity. Perhaps: "The predicate can be either a string lambda (see $(XREF functional,binaryFun) for more details) or an alias that can be called with $(D pred(element, s))"

@JackStouffer JackStouffer force-pushed the lambda branch 3 times, most recently from a591749 to 8a62e77 Compare November 30, 2015 15:59
@JackStouffer
Copy link
Member Author

@schveiguy fixed.

@schveiguy
Copy link
Member

Looks better. On the second splitter overload, it looks like the predicate must accept an element of r and an element of s, so the blurb about what predicates are accepted may need to be reworded, or maybe we can come up with a more "portable" way of saying it.

@JackStouffer
Copy link
Member Author

@schveiguy Ok. But on the whole is this a good compromise? Can I start applying this to the other functions?

@schveiguy
Copy link
Member

It's fine with me, I cannot speak for others who haven't looked at it yet.

@JackStouffer
Copy link
Member Author

Updated the rest of the docs.

@JackStouffer
Copy link
Member Author

ping

@quickfur
Copy link
Member

This has been brought up before. Some people support this, some don't. Maybe @andralex should make a judgment call here and settle this issue once and for all, instead of letting this PR bit-rot?

@andralex
Copy link
Member

Yah, we need to fix function equality in the future but string lambdas should be slowly and surely phased out. @JackStouffer thx for the work, when modifying comments in the future please take the opportunity to replace $(D ...) with backquotes.

@andralex
Copy link
Member

Auto-merge toggled on

@quickfur
Copy link
Member

Function equality is, in the general case, undecidable. However, for our purposes, equality for most of the function literals we'll need to compare in normal use cases can probably be sufficiently approximated by some kind of AST structure matching. It won't catch all the cases, but it will certainly catch the cases that string lambdas can catch, and many more besides. (E.g., currently a==b, a == b, a ==b, a== b, are all distinct string lambdas, but the corresponding function literals would be identified as equal by AST comparison.)

andralex added a commit that referenced this pull request Jan 16, 2016
Removed string predicates from std.algorithm.iteration documentation
@andralex andralex merged commit 731e47e into dlang:master Jan 16, 2016
@JackStouffer
Copy link
Member Author

Thanks!

@andralex
Copy link
Member

@quickfur yah, alpha renaming too. Someone should write a DIP for this...

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.

4 participants