Skip to content

Conversation

w0rp
Copy link
Contributor

@w0rp w0rp commented May 2, 2013

This address an issue with parse functions for numeric values not supporting r-value ranges. By fixing this, it makes it possible to use parse on a slice of an std.container Array.

…o ref parameters for the ranges. Unit tests have been added to match each modification.
@w0rp
Copy link
Contributor Author

w0rp commented May 2, 2013

My knowledge of std.conv is quite limited, so comments on this patch, perhaps involving the different variations of parse I'm not familiar with, are very welcome.

@@ -24,6 +24,10 @@ import std.algorithm, std.array, std.ascii, std.exception, std.math, std.range,
std.utf;
import std.format;

version(unittest) {
import std.container;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should be able to import this directly inside the rel event unittest bloc. It limits the scope more.

@w0rp
Copy link
Contributor Author

w0rp commented May 4, 2013

I was wondering if there would be any damaging consequences to making all variations of parse auto ref for the input range. Should I just go ahead and do it?

I'll move the imports inside of the unit test blocks themselves. This is a style choice I've been wondering about.

Target parse(Target, Source)(ref Source s)
if (isSomeChar!(ElementType!Source) &&
Target parse(Target, Source)(auto ref Source s)
if (isInputRange!Source && isSomeChar!(ElementType!Source) &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ElementType!Source does not require that Source is a range, yet Source has to be a range as a popFront function is required in the body of these parse functions. Thus, isInputRange!Source.

Copy link
Member

Choose a reason for hiding this comment

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

Yah, I agree with the others that auto ref here does more harm than good. Could you please remove that from all overloads? The changes to the constraints seem legit.

@w0rp
Copy link
Contributor Author

w0rp commented May 4, 2013

After some consideration, I have taken a different approach to the fix, and now the parse functions should work with any input range, regardless of whether or not the input range is an l-value or r-value. There are some exceptions to this. It will not work with straight input ranges on enum types, bool, or typeof(null), as these variations of parse require slice operations. I don't think I'm clever enough to fix this.

parse with other types ought to work, like numbers, dynamic/static arrays of numbers, and associative arrays (like int[string]).

@monarchdodra
Copy link
Collaborator

Strange, I thought I had commented on this before. Anyways:

I don't think parse should accept R-values. While it might look convenient on paper, the modifications that parse deals to source must be inspected for proper validation.

For example:parse!double("12,5") will work, but will stop parsing at ",5", which may not have been intended.

If the user does not want to validate the output of parse, then that's the user's problem, but the interface of parse itself should not promote it. Further more, it can be bug prone if the user does think his range gets consumed, eg:

while (!s.null)
    writeln(s[].parse!int);

That little inconspicuous [] is all it takes to break the code, and turn that snippet into a never ending loop. The bug that could have been caught compile time, is now in run-time.

FYI, I had submitted the same thing for formattedRead (#777), and it was turned down for the same reasons, which I now agree with.

@jmdavis
Copy link
Member

jmdavis commented Jun 5, 2013

It would be incredibly harmful IMHO for parse to accept rvalues. It requires lvalues by design. Maybe Kenji ( @9nsr ) will have something to say about this, but I'm very much inclined to just close both this and issue# 10019 as invalid.

@w0rp w0rp closed this Jun 13, 2013
@w0rp
Copy link
Contributor Author

w0rp commented Jun 13, 2013

I'll just close this down for doing more harm than good.

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