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

Issue 5236 - raw reading for integers and a few refactorings #4912

Merged
merged 8 commits into from
Dec 7, 2016

Conversation

RazvanN7
Copy link
Collaborator

Added raw reading for integers

@dlang-bot
Copy link
Contributor

dlang-bot commented Nov 18, 2016

Fix Bugzilla Description
5236 [patch] std.format.formattedRead/unformatValue does not support the raw reading of integer types

@RazvanN7 RazvanN7 changed the title Bootcamp fixes Issue 5236 Nov 18, 2016
Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

Getting there!

@@ -4300,9 +4300,9 @@ T unformatValue(T, Range, Char)(ref Range input, ref FormatSpec!Char spec)
{
import std.algorithm.searching : find;
import std.conv : parse, text;
if (spec.spec == 's')
static if (is(typeof(parse!T(input)) == T))
Copy link
Member

Choose a reason for hiding this comment

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

So here T is always a (possibly qualified) bool. Under what circumstances may parse!T(input) not compile?

{
static if (isSomeString!Range)
{
x.raw[i] = input[0];
Copy link
Member

Choose a reason for hiding this comment

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

This will not compile if Range is a wide string: you'd attempt to assign a wide char (2 or 4 bytes) over a single byte. I suggest you change the constraint for rawRead to just ElementType!(Range).sizeof == 1. There is a crazier possibility by which you do accept wide strings and you do implement raw reads, but only if T.sizeof is a multiple of the character size. Probably we don't need this; most people who'd entertain the idea of a raw read will do so from a stream of bytes or a narrow string.

else
{
// TODO: recheck this
x.raw[i] = cast(ubyte) input.front;
Copy link
Member

Choose a reason for hiding this comment

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

To avoid this cast and clarify the possibilities, you may put this constraint for the function:

if (is(Unqual!(ElementType!Range) == char) || is(Unqual!(ElementType!Range) == byte) || is(Unqual!(ElementType!Range) == ubyte))

import std.algorithm.searching : find;
import std.conv : parse, text;

if (spec.spec == 'r') return rawRead!T(input);
Copy link
Member

Choose a reason for hiding this comment

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

Following the logic before, here if somebody wants to parse a wide string with %r you would issue a run-time error:

if (spec.spec == 'r')
{
    static if (is(Unqual!(ElementType!Range) == char) || is(Unqual!(ElementType!Range) == byte) || is(Unqual!(ElementType!Range) == ubyte))
        return rawRead!T(input);
    else
        throw new Exception("The raw read specifier %r may only be used with narrow strings and ranges of bytes."); 
}

}
return x.typed;
}

/**
Reads an integral value and returns it.
*/
T unformatValue(T, Range, Char)(ref Range input, ref FormatSpec!Char spec)
if (isInputRange!Range && isIntegral!T && !is(T == enum))
Copy link
Member

Choose a reason for hiding this comment

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

This is a distinct matter prior to this PR: the constraint here is too lax, e.g. I could call unformatValue with a float[] and will fail to compile somewhere in the innards of the implementation. The function should recuse itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry but I don't get it. All of the other overload functions for unformat value test that T is of a certain type and that input is an InputRange. Why whould the routine for integrals be different?

Copy link
Member

Choose a reason for hiding this comment

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

Consider this code:

void main()
{
	import std.format;
	double[] array;
	FormatSpec!char sp;
	auto x = unformatValue!int(array, sp);
}

This fails to compile (try it!), and the error messages point inside the standard library code. We never want to allow that to happen. Instead, we want to point at the user code "You passed the wrong arguments". That's why we need all templates in the stdlib have constraints that describe exactly the inputs they support.

There are clearly still places where existing code does not satisfy this, but at least we should make sure new code has the appropriate constraints. You may want to fix the problem for the other overloads as well!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, if I understand correctly, the only case where "auto x = unformatValue!int(array, sp);" should compile is when isSomeChar!(ElementType!<array_Type>)?

@ntrel
Copy link
Contributor

ntrel commented Nov 25, 2016

@RazvanN7 Could you include a description in the title of your PRs please - thanks! It helps with managing the PR queue vs somehow remembering bug numbers.

@RazvanN7 RazvanN7 changed the title Issue 5236 Issue 5236 - raw reading for integers and a few refactorings Nov 28, 2016
static if (is(Unqual!(ElementEncodingType!Range) == char)
|| is(Unqual!(ElementEncodingType!Range) == byte)
|| is(Unqual!(ElementEncodingType!Range) == ubyte))
return rawRead!T(input);
Copy link
Member

Choose a reason for hiding this comment

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

unindent one level

Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

OK, we're good modulo minute nits. Thanks!

static if (is(Unqual!(ElementEncodingType!Range) == char)
|| is(Unqual!(ElementEncodingType!Range) == byte)
|| is(Unqual!(ElementEncodingType!Range) == ubyte))
return rawRead!T(input);
Copy link
Member

Choose a reason for hiding this comment

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

unindent one level

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. Thank you

@andralex
Copy link
Member

andralex commented Dec 7, 2016

Auto-merge toggled on

@andralex andralex merged commit 9939897 into dlang:master Dec 7, 2016
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