Skip to content

Fix Issue 10662 - byLine!(Char, immutable Char) won't compile #1418

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

Closed
wants to merge 2 commits into from

Conversation

ntrel
Copy link
Contributor

@ntrel ntrel commented Jul 17, 2013

http://d.puremagic.com/issues/show_bug.cgi?id=10662

This wouldn't compile because readln(buf) now requires a mutable buf.
Also make ByLine.line, ByLine.first_call private.

@@ -1098,7 +1116,7 @@ Allows to directly use range operations on lines of a file.
popFront();
first_call = false;
}
return line;
return to!(Char[])(line);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just realized this will allocate a new string on each call to front.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But if you don't allocate a new string, that basically means you are going to clobber the returned string...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's why I did it, but I think repeated calls to front without calling popFront shouldn't keep making copies. Will fix.

@ntrel
Copy link
Contributor Author

ntrel commented Jul 17, 2013

repeated calls to front without calling popFront shouldn't keep making copies

Now fixed.

@@ -1117,6 +1136,8 @@ Allows to directly use range operations on lines of a file.
{
line = line.ptr[0 .. line.length - 1];
}
// duplicate if necessary
_front = to!(Char[])(line);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now fixed

Except now, you'll trigger a dupe even for normal ByLine!char. Also, you might as well just call .dup followed by a cast here. Who knows what to! is doing (or not...) behind the scenes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. From http://dlang.org/phobos/std_conv.html#.to :

Converting a value to its own type (useful mostly for generic code) simply returns its argument.

@monarchdodra :

you might as well just call .dup followed by a cast here

I would need a static if. I think to is better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hum.... That might warrant an extra comment (IMO). But OK, works for me.

Copy link
Member

Choose a reason for hiding this comment

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

Yah, this is an intended use case for to - just use it and it'll do the right thing.

@monarchdodra
Copy link
Collaborator

I'm not sure I agree with this "fix" (I explained why in your bug report). Having the behavior depend on the qualification of the input is weird for me: I think it would warrant a newly named function. If I understand you right, you are trying to do:

char => reusable buffer: OK
immutable(char) => duped before returning buffer: I guess... why not?
const(char) => ???

I'm leaning in favor leaving ByLine "as is" (we can add some constraints to check for mutable types if necessary), and instead introduce a byDupLine function. This function would dup, regardless of type (be it char or immutable char). This would mean having a "standard and not surprising behavior". I also think having a byLine that doesn't clobber your data, be it mutable or not, to be something very good to have (this has been requested for repeatedly).

This wouldn't compile because readln(buf) now requires a mutable buf.
Make ByLine.line, ByLine.first_call private.
Update unittest to test non-empty files without any newlines.
@ntrel
Copy link
Contributor Author

ntrel commented Jul 18, 2013

Changed to use Unqual, updated unittest, squashed commits.

As mentioned on the bug tracker, I'll probably start a discussion about improving byLine or alternatives to byLine in a few days, but I think this fix is still sensible.

@ntrel
Copy link
Contributor Author

ntrel commented Jul 22, 2013

I'll probably start a discussion about improving byLine or alternatives

See http://forum.dlang.org/post/ksj7b6$86b$1@digitalmars.com

@ntrel
Copy link
Contributor Author

ntrel commented Jul 23, 2013

Closed as monarchdodra and jmdavis are opposed, plus readText.splitter is probably more efficient than this.

@ntrel ntrel closed this Jul 23, 2013
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.

3 participants