Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

#8729 fixes #817

Closed
wants to merge 2 commits into from

4 participants

@monarchdodra
Collaborator

This PR contains:

  1. 8729: [parse|to]!double should NOT accept " 123.5"
  2. Typo: "case-insensive" => "case-insensitive"
  3. Proper checking in parse array: These boldly accessed the front of the range, without checking for elements: This created asserts, when parse promises to throw a ConvException.

Anything regarding skip white has now been removed from this pull (which is now just a bug fix).

skipWhite is now a standalone ER @ 827 #827

@monarchdodra monarchdodra commented on the diff
std/conv.d
@@ -1711,8 +1727,8 @@ Target parse(Target, Source)(ref Source s)
else
{
// Larger than int types
- if (s.empty)
- goto Lerr;
@monarchdodra Collaborator

This test was not needed, as the code naturally handles empty. No point in paying every time for a case which should rarilly happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
std/conv.d
@@ -2719,7 +2766,8 @@ unittest
private void skipWS(R)(ref R r)
{
- skipAll(r, ' ', '\n', '\t', '\r');
+ for ( ; !r.empty && std.uni.isWhite(r.front) ; r.popFront())
+ { }
@monarchdodra Collaborator

This code was changed because it did not properly support unicode whitespaces (such as the japanese double width hiragana space and whatnot).

It may be (arguably) more efficient to boot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@9rnsr
Collaborator

This is incorrect change. The parse family should not skip leading white spaces implicitly.
So, just only parse!double should be fixed.

@9rnsr
Collaborator

There are some reasons:

  • It's based on unix philosophy: "Write programs that do one thing and do it well". It's really bad that "some parse functions may remove the leading WS, others doesn't".
  • The definition of whitespace depends on the application work. If you write a text processor, skipping "fullwidth whitespace(U+3000)" would be expected, but if you write a D source code parser, it's not expected (See here).
  • With UFCS, you can express the code behavior like follows:
string input = "..."
int n = input.parse!int();
double f = input.skipWhiteSpace.parse!double();
// parse double after removing leading whitespaces
@ghost

That's input.stripLeft.parse.. btw.

@9rnsr
Collaborator

That's input.stripLeft.parse.. btw.

It's not so bad. But, unfortunately, std.string.stripLeft doesn't work for character ranges, just for [wd]?string.

@monarchdodra
Collaborator

OK, 3 things:

  1. I'll rollback the change and fix floating point type case. I'll also investigate the array parser, which I think may also be skiping leading ws.
  2. I'll add a documentation note, as there is apparently some confusion regarding proper behavior of parse. (along with recommended ways of skipping ws)
  3. Regarding the definition of "ws": I thought parse was meant as an end user function, not an internal parser...? Was my change in "skipWS" wrong, or just arguable?
@monarchdodra
Collaborator

Technically:
double f = input.stripLeft.parse!double();
Will not work, because stripLeft will take and return by value, so:
1. parse will reffuse to bind a ref to a temporary
2. even if it did, the original input would not be modified.

Nothing a 2 liner can't fix of course.

@9rnsr
Collaborator

Regarding the definition of "ws": I thought parse was meant as an end user function, not an internal parser...? Was my change in "skipWS" wrong, or just arguable?

skipWS is just used for compound literal parsing: array literal and associative array literal.
Then, it is wrong to me that parsing the string "[1, 2,\u30003]" to [1,2,3] by parse!(int[]).

@andralex
Owner

The reason for the rigid design or parse is exactly as @9rnsr mentioned. My intent was to have 100% control over the parsed format if there's a need for it. However, it's also true that most often you do want to skip whitespace. So maybe my design was wrong because I made the less common case the default. I'm thinking it would be okay to skip whitespace by default. People who do NOT want to skip whitespace can look at r.front and see if it's a whitespace. That's more work for them, but probably it's a rare case. Thoughts?

@9rnsr
Collaborator

I'm thinking it would be okay to skip whitespace by default.

Maybe it is true, and I can agree with it. But, I'm worried about parse!SomeChar (code).

It strips just one leading character from the given input. If we introduce the skip leading WS behavior, it will become just one overload which has a special behavior? Or will become to strip one or more characters?
In either case, the inconsistent behavior among parse overloads will be introduced.

@monarchdodra
Collaborator

skipWS is just used for compound literal parsing: array literal and associative array literal.

True, I though about that during the night. Good point.

Maybe it is true, and I can agree with it. But, I'm worried about parse!SomeChar.

It strips just one leading character from the given input. If we introduce the skip leading WS behavior, it will become just one overload which has a special behavior? Or will become to strip one or more characters?

Arguably, I'd expect parse!SomeChar to extract the first non-ws, and strip one or more characters. If "parse" is designed to work anything like C++'s stream parsing (as I thought it did, and apparently, Jonathan M Davis too), then ws is stripped away, including for chars:

std::stringstream ss("123    a");
int i;
char a;
ss >> i >> a;
assert(i == 123);
assert(a == 'a');

In either case, the inconsistent behavior among parse overloads will be introduced.

Actually, that would be consistent behavior, no?

On a side note, your point highlights that my fix was not "complete" because I did not do anything for parse!SomeChar. Gonna fix that now.

My proposal: I'm going to finish this PR, for full support of skip ws parsing, because I already started it. We can then discuss the change in the forums?

In the meantime, I'll also do a clean simple fix for no-strip with doubles (which also has the 1-2 issues I found).

@jmdavis
Collaborator

I thought that parse did skip whitespace, and it does appear to when parsing integers and floats. So, it looks like it's current behavior is inconsistent regardless of what it should be doing. Personally, I think that it would be most useful if it strip leading whitespace for you so that you can parse without worrying about whitespace but also parse with caring about it, because you can explicitly handle the whitespace after parsing a value (since it would be stripped from in front of a value when parsing it). The one downside to that approach that I can think of is that if you were ignoring whitespace and the string ended with whitespace and you were parsing as long as the string wasn't empty, you'd end up trying to parse whitespace and presumably end up with an exception. It could be solved by making parse configurable with regards to whether it strips whitespace or not, but I don't know if that's complicating things too much or not.

@monarchdodra
Collaborator

The one downside to that approach that I can think of is that if you were ignoring whitespace and the string ended with whitespace and you were parsing as long as the string wasn't empty, you'd end up trying to parse whitespace and presumably end up with an exception.

You know, that is a very good point actually. I don't think there is a good solution either.

It could be solved by making parse configurable with regards to whether it strips whitespace or not, but I don't know if that's complicating things too much or not.

I think that would be very complicated.


I had thought of a solution I hadn't mentioned yet, but I'm starting to think that maybe just adding a "parseWhite" function which would take and return by ref would be a simple, convenient, low impact and configurable fix, all in one!

string ss = "123 12.5"
int i = ss.parseWhite().parse!double();
double d = ss.parseWhite().parse!double();

or

string ss = "..."
while(!ss.parseWhite().empty())
    writeln(ss.parse!int());

I think it would be a really good solution. Thoughts?

PS: This would NOT be duplication with stripLeft because...
1. Strip left only operates on natural strings, but not generic range of characters, such as cycle!(char[])
2. Strip left can't be ufcs chained, and does not modify by ref, which would make it verboselly inconvenient.

@monarchdodra monarchdodra commented on the diff
std/conv.d
((8 lines not shown))
+ foreach(i, dchar c; r)
+ {
+ if(!std.ascii.isWhite(c))
+ {
+ r = r[i .. $];
+ return;
+ }
+ }
+ r = r[0 .. 0]; //Empty string with correct type.
+ return;
+ }
+ else
+ {
+ for ( ; !r.empty && std.ascii.isWhite(r.front) ; r.popFront())
+ { }
+ }
}
@monarchdodra Collaborator

Arguably more efficient in all cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@monarchdodra monarchdodra commented on the diff
std/conv.d
@@ -2788,6 +2838,14 @@ unittest
assert(a2 == ["aaa", "bbb", "ccc"]);
}
+unittest
+{
+ //Check proper failure
+ auto ss = "[ 1 , 2 , 3 ]";
+ foreach(i ; 0..ss.length-1)
+ assertThrown!ConvException(parse!(int[])(ss[0 .. i]));
+}
@monarchdodra Collaborator

!Important test!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@monarchdodra
Collaborator

Done! Thoughts?

@9rnsr
Collaborator

I like this direction, except one point: it is the name of parseWhite.
It just skip leading white spaces instead of parse them, because parseWhite does not return parsed them.
So, I think that skipWhite is better name than it..

@monarchdodra
Collaborator

I like this direction, except one point: it is the name of parseWhite.
It just skip leading white spaces instead of parse them, because parseWhite does not return parsed them.
So, I think that skipWhite is better name than it..

I had the exact same thought, but at the same time, I would have liked the word "parse" to appear in the function name... :/

skipWhite is probably better though.

@monarchdodra
Collaborator

Per the "1 change per pull", I removed the skipWhite dev from this PR. Please see #827 instead

@monarchdodra monarchdodra reopened this
@monarchdodra monarchdodra referenced this pull request
Merged

#8729 fixes #828

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 21, 2012
  1. @monarchdodra

    Consolidating Array

    monarchdodra authored
Commits on Oct 2, 2012
  1. @monarchdodra

    #8729 fixes

    monarchdodra authored
This page is out of date. Refresh to see the latest.
Showing with 188 additions and 134 deletions.
  1. +131 −121 std/container.d
  2. +57 −13 std/conv.d
View
252 std/container.d
@@ -2063,6 +2063,9 @@ Array type with deterministic control of memory. The memory allocated
for the array is reclaimed as soon as possible; there is no reliance
on the garbage collector. $(D Array) uses $(D malloc) and $(D free)
for managing its own memory.
+
+Array can be provide a $(D RandomAccessRange) view of itself with $(D opSlice).
+Accessing or modifying elements via that range is slightly faster.
*/
struct Array(T) if (!is(T : const(bool)))
{
@@ -2217,6 +2220,17 @@ struct Array(T) if (!is(T : const(bool)))
private alias RefCounted!(Payload, RefCountedAutoInitialize.no) Data;
private Data _data;
+ private @property T[] _get()
+ {
+ static T[] _emptyPayload;
+ return _data.RefCounted.isInitialized ? _data._payload : _emptyPayload;
+ }
+ private @property const(T[]) _get() const
+ {
+ static T[] _emptyPayload;
+ return _data.RefCounted.isInitialized ? _data._payload : _emptyPayload;
+ }
+
this(U)(U[] values...) if (isImplicitlyConvertible!(U, T))
{
auto p = cast(T*) malloc(T.sizeof * values.length);
@@ -2253,32 +2267,47 @@ Defines the container's primary range, which is a random-access range.
*/
struct Range
{
- private Array _outer;
+ private Data _data;
private size_t _a, _b;
- this(Array data, size_t a, size_t b)
+ this(Data data, size_t a, size_t b)
{
- _outer = data;
+ _data = data;
_a = a;
_b = b;
+
+ //Invariant: _data is always initialized.
+ //This gives the ranges more power.
+ _data.RefCounted.ensureInitialized();
+ //If _data was already initialized, no change
+ //If it wasn't, then the underlying Array and Range don't reference each other anyways
+ }
+
+ @property T[] _get()
+ {
+ assert(_b <= _data._payload.length, "Array.Range: Range has been invalidated");
+ return _data._payload[_a .. _b];
+ }
+ @property const(T[]) _get() const
+ {
+ assert(_b <= _data._payload.length, "Array.Range: Range has been invalidated");
+ return _data._payload[_a .. _b];
}
@property Range save()
{
- assert(_b <= _outer.length);
+ assert(_get);
return this;
}
@property bool empty() const
{
- assert(_b <= _outer.length);
- return _a >= _b;
+ return _get.empty;
}
@property size_t length() const
{
- assert(_b <= _outer.length);
- return _b - _a;
+ return _get.length;
}
size_t opDollar() const
@@ -2288,159 +2317,140 @@ Defines the container's primary range, which is a random-access range.
@property T front()
{
- enforce(!empty);
- return _outer[_a];
+ return _get.front;
}
@property T back()
{
- enforce(!empty);
- return _outer[_b - 1];
+ return _get.back;
}
@property void front(T value)
{
- enforce(!empty);
- _outer[_a] = move(value);
+ move(value, _get.front);
}
@property void back(T value)
{
- enforce(!empty);
- _outer[_b - 1] = move(value);
+ move(value, _get.back);
}
void popFront()
+ in
+ {
+ auto dummy = _get();
+ dummy.popFront();
+ }
+ body
{
- enforce(!empty);
++_a;
}
void popBack()
+ in
+ {
+ auto dummy = _get();
+ dummy.popBack();
+ }
+ body
{
- enforce(!empty);
--_b;
}
T moveFront()
{
- enforce(!empty);
- return move(_outer._data._payload[_a]);
+ return move(_get.front);
}
T moveBack()
{
- enforce(!empty);
- return move(_outer._data._payload[_b - 1]);
+ return move(_get.back);
}
T moveAt(size_t i)
{
- i += _a;
- enforce(i < _b && !empty);
- return move(_outer._data._payload[i]);
+ return move(_get[i]);
}
T opIndex(size_t i)
{
- i += _a;
- enforce(i < _b && _b <= _outer.length);
- return _outer._data._payload[i];
+ return _get[i];
}
void opIndexUnary(string op)(size_t i)
if(op == "++" || op == "--")
{
- i += _a;
- enforce(i < _b && _b <= _outer.length);
- mixin(op~"_outer._data._payload[i];");
+ mixin(op~"_get[i];");
}
T opIndexUnary(string op)(size_t i)
if(op != "++" && op != "--")
{
- i += _a;
- enforce(i < _b && _b <= _outer.length);
- mixin("return "~op~"_outer._data._payload[i];");
+ mixin("return "~op~"_get[i];");
}
void opIndexAssign(T value, size_t i)
{
- i += _a;
- enforce(i < _b && _b <= _outer.length);
- _outer[i] = value;
+ move(value, _get[i]);
}
void opIndexOpAssign(string op)(T value, size_t i)
{
- i += _a;
- enforce(i < _b && _b <= _outer.length);
- mixin("_outer._data._payload[i] "~op~"= value;");
+ mixin("_get[i] "~op~"= value;");
}
typeof(this) opSlice()
+ in
+ {
+ auto dummy = _get;
+ }
+ body
{
- assert(_b <= _outer.length);
return this;
}
typeof(this) opSlice(size_t a, size_t b)
+ in
+ {
+ auto dummy = _get[a .. b];
+ }
+ body
{
- assert(_b <= _outer.length);
a += _a;
b += _a;
- enforce(a <= b && b <= _b);
- return typeof(this)(_outer, a, b);
+ return typeof(this)(_data, a, b);
}
void opSliceAssign(T value)
{
- assert(_b <= _outer.length);
- _outer._data._payload[_a .. _b] = value;
+ _get[] = value;
}
void opSliceAssign(T value, size_t i, size_t j)
{
- assert(_b <= _outer.length);
- if(i == 0 && j == 0 ) return;
- i += _a;
- j += _a;
- enforce(i <= j && j <= _b);
- _outer._data._payload[i .. j] = value;
+ _get[i .. j] = value;
}
void opSliceUnary(string op)()
if(op == "++" || op == "--")
{
- assert(_b <= _outer.length);
- mixin(op~"_outer._data._payload[_a .. _b];");
+ mixin(op~"_get[];");
}
void opSliceUnary(string op)(size_t i, size_t j)
if(op == "++" || op == "--")
{
- assert(_b <= _outer.length);
- if(i == 0 && j == 0 ) return;
- i += _a;
- j += _a;
- enforce(i <= j && j <= _b);
- mixin(op~"_outer._data._payload[i .. j];");
+ mixin(op~"_get[i .. j];");
}
void opSliceOpAssign(string op)(T value)
{
- assert(_b <= _outer.length);
- mixin("_outer._data._payload[_a .. _b] "~op~"= value;");
+ mixin("_get[] "~op~"= value;");
}
void opSliceOpAssign(string op)(T value, size_t i, size_t j)
{
- assert(_b <= _outer.length);
- if(i == 0 && j == 0 ) return;
- i += _a;
- j += _a;
- enforce(i <= j && j <= _b);
- mixin("_outer._data._payload[i .. j] "~op~"= value;");
+ mixin("_get[i .. j] "~op~"= value;");
}
}
@@ -2464,7 +2474,7 @@ Complexity: $(BIGOH 1)
*/
@property bool empty() const
{
- return !_data.RefCounted.isInitialized || _data._payload.empty;
+ return _get.empty;
}
/**
@@ -2474,7 +2484,7 @@ Complexity: $(BIGOH 1).
*/
@property size_t length() const
{
- return _data.RefCounted.isInitialized ? _data._payload.length : 0;
+ return _get.length;
}
/// ditto
@@ -2492,6 +2502,7 @@ Complexity: $(BIGOH 1)
*/
@property size_t capacity()
{
+ //Do NOT use _get here, as .capacity will lie
return _data.RefCounted.isInitialized ? _data._capacity : 0;
}
@@ -2531,8 +2542,8 @@ Complexity: $(BIGOH 1)
Range opSlice()
{
// Workaround for bug 4356
- Array copy;
- copy._data = this._data;
+ Data copy;
+ copy = this._data;
return Range(copy, 0, length);
}
@@ -2548,8 +2559,8 @@ Complexity: $(BIGOH 1)
{
enforce(a <= b && b <= length);
// Workaround for bug 4356
- Array copy;
- copy._data = this._data;
+ Data copy;
+ copy = this._data;
return Range(copy, a, b);
}
@@ -2562,29 +2573,25 @@ Complexity: $(BIGOH 1)
*/
@property T front()
{
- enforce(!empty);
- return *_data._payload.ptr;
+ return _get.front;
}
/// ditto
@property void front(T value)
{
- enforce(!empty);
- *_data._payload.ptr = value;
+ move(value, _get.front);
}
/// ditto
@property T back()
{
- enforce(!empty);
- return _data._payload[$ - 1];
+ return _get.back;
}
/// ditto
@property void back(T value)
{
- enforce(!empty);
- _data._payload[$ - 1] = value;
+ move(value, _get.back);
}
/**
@@ -2596,38 +2603,33 @@ Complexity: $(BIGOH 1)
*/
T opIndex(size_t i)
{
- enforce(_data.RefCounted.isInitialized);
- return _data._payload[i];
+ return _get[i];
}
/// ditto
void opIndexUnary(string op)(size_t i)
if(op == "++" || op == "--")
{
- enforce(_data.RefCounted.isInitialized);
- mixin(op~"_data._payload[i];");
+ mixin(op~"_get[i];");
}
/// ditto
T opIndexUnary(string op)(size_t i)
if(op != "++" && op != "--")
{
- enforce(_data.RefCounted.isInitialized);
- mixin("return "~op~"_data._payload[i];");
+ mixin("return "~op~"_get[i];");
}
/// ditto
void opIndexAssign(T value, size_t i)
{
- enforce(_data.RefCounted.isInitialized);
- _data._payload[i] = value;
+ move(value, _get[i]);
}
/// ditto
void opIndexOpAssign(string op)(T value, size_t i)
{
- enforce(_data.RefCounted.isInitialized);
- mixin("_data._payload[i] "~op~"= value;");
+ mixin("_get[i] "~op~"= value;");
}
/**
@@ -2640,43 +2642,37 @@ Complexity: $(BIGOH slice.length)
void opSliceAssign(T value)
{
- if(!_data.RefCounted.isInitialized) return;
- _data._payload[] = value;
+ _get[] = value;
}
void opSliceAssign(T value, size_t i, size_t j)
{
- enforce(_data.RefCounted.isInitialized || (i == 0 && j == 0));
- _data._payload[i .. j] = value;
+ _get[i .. j] = value;
}
void opSliceUnary(string op)()
if(op == "++" || op == "--")
{
- if(!_data.RefCounted.isInitialized) return;
- mixin(op~"_data._payload[];");
+ mixin(op~"_get[];");
}
/// ditto
void opSliceUnary(string op)(size_t i, size_t j)
if(op == "++" || op == "--")
{
- enforce(_data.RefCounted.isInitialized || (i == 0 && j == 0));
- mixin(op~"_data._payload[i .. j];");
+ mixin(op~"_get[i .. j];");
}
/// ditto
void opSliceOpAssign(string op)(T value)
{
- if(!_data.RefCounted.isInitialized) return;
- mixin("_data._payload[] "~op~"= value;");
+ mixin("_get[] "~op~"= value;");
}
/// ditto
void opSliceOpAssign(string op)(T value, size_t i, size_t j)
{
- enforce(_data.RefCounted.isInitialized || (i == 0 && j == 0));
- mixin("_data._payload[i .. j] "~op~"= value;");
+ mixin("_get[i .. j] "~op~"= value;");
}
/**
@@ -2726,7 +2722,14 @@ Complexity: $(BIGOH n)
*/
void clear()
{
- .destroy(_data);
+ //Do not destroy _data outright, as the ranges still need it.
+ //Destroy the individual elements, then "detach"
+ if(_data.RefCounted.isInitialized)
+ {
+ _data.length = 0;
+ _data = Data();
+ }
+ //.destroy(_data);
}
/**
@@ -2825,6 +2828,7 @@ Complexity: $(BIGOH howMany).
*/
size_t removeBack(size_t howMany)
{
+ if(!_data.RefCounted.isInitialized) return 0;
if (howMany > length) howMany = length;
static if (is(T == struct))
{
@@ -2854,7 +2858,7 @@ Complexity: $(BIGOH n + m), where $(D m) is the length of $(D stuff)
size_t insertBefore(Stuff)(Range r, Stuff stuff)
if (isImplicitlyConvertible!(Stuff, T))
{
- enforce(r._outer._data is _data && r._a <= length);
+ enforce(ownsRange(r), "Array.insertBefore: Range does not belong to the Array");
reserve(length + 1);
assert(_data.RefCounted.isInitialized);
// Move elements over by one slot
@@ -2870,7 +2874,7 @@ Complexity: $(BIGOH n + m), where $(D m) is the length of $(D stuff)
size_t insertBefore(Stuff)(Range r, Stuff stuff)
if (isInputRange!Stuff && isImplicitlyConvertible!(ElementType!Stuff, T))
{
- enforce(r._outer._data is _data && r._a <= length);
+ enforce(ownsRange(r), "Array.insertBefore: Range does not belong to the Array");
static if (isForwardRange!Stuff)
{
// Can find the length in advance
@@ -2907,7 +2911,7 @@ Complexity: $(BIGOH n + m), where $(D m) is the length of $(D stuff)
/// ditto
size_t insertAfter(Stuff)(Range r, Stuff stuff)
{
- enforce(r._outer._data is _data);
+ enforce(ownsRange(r), "Array.insertAfter: Range does not belong to the Array");
// TODO: optimize
immutable offset = r._b;
enforce(offset <= length);
@@ -2921,7 +2925,7 @@ Complexity: $(BIGOH n + m), where $(D m) is the length of $(D stuff)
size_t replace(Stuff)(Range r, Stuff stuff)
if (isInputRange!Stuff && isImplicitlyConvertible!(ElementType!Stuff, T))
{
- enforce(r._outer._data is _data);
+ enforce(ownsRange(r), "Array.replace: Range does not belong to the Array");
size_t result;
for (; !stuff.empty; stuff.popFront())
{
@@ -2943,7 +2947,7 @@ Complexity: $(BIGOH n + m), where $(D m) is the length of $(D stuff)
size_t replace(Stuff)(Range r, Stuff stuff)
if (isImplicitlyConvertible!(Stuff, T))
{
- enforce(r._outer._data is _data);
+ enforce(ownsRange(r), "Array.replace: Range does not belong to the Array");
if (r.empty)
{
insertBefore(r, stuff);
@@ -2971,9 +2975,7 @@ $(D r)
*/
Range linearRemove(Range r)
{
- enforce(r._outer._data is _data);
- enforce(_data.RefCounted.isInitialized);
- enforce(r._a <= r._b && r._b <= length);
+ enforce(ownsRange(r), "Array.linearRemove: Range does not belong to the Array");
immutable offset1 = r._a;
immutable offset2 = r._b;
immutable tailLength = length - offset2;
@@ -2984,6 +2986,14 @@ $(D r)
}
/// ditto
alias remove stableLinearRemove;
+
+ //A "good enough" test of range ownership
+ private bool ownsRange(Range r)
+ {
+ assert(r._b <= r._data.length, "Array.Range: Range has been invalidated");
+ return (r._data is _data) || //First test is redundant, but cheaper
+ (empty && r.empty);
+ }
}
// unittest
@@ -3187,14 +3197,14 @@ unittest
// make sure that Array instances refuse ranges that don't belong to them
unittest
{
- Array!int a = [1, 2, 3];
- auto r = a.dup[];
- assertThrown(a.insertBefore(r, 42));
- assertThrown(a.insertBefore(r, [42]));
- assertThrown(a.insertAfter(r, 42));
- assertThrown(a.replace(r, 42));
- assertThrown(a.replace(r, [42]));
- assertThrown(a.linearRemove(r));
+ Array!int a = [1, 2, 3];
+ auto r = a.dup[];
+ assertThrown(a.insertBefore(r, 42));
+ assertThrown(a.insertBefore(r, [42]));
+ assertThrown(a.insertAfter(r, 42));
+ assertThrown(a.replace(r, 42));
+ assertThrown(a.replace(r, [42]));
+ assertThrown(a.linearRemove(r));
}
unittest
{
View
70 std/conv.d
@@ -68,6 +68,7 @@ private void parseError(lazy string msg, string fn = __FILE__, size_t ln = __LIN
private void parseCheck(alias source)(dchar c, string fn = __FILE__, size_t ln = __LINE__)
{
+ if(source.empty) parseError(text("unexpected end of input when expecting", "\"", c, "\""));
if (source.front != c)
parseError(text("\"", c, "\" is missing"), fn, ln);
source.popFront();
@@ -134,7 +135,7 @@ class ConvOverflowException : ConvException
}
}
-/* **************************************************************
+/**
The $(D_PARAM to) family of functions converts a value from type
$(D_PARAM Source) to type $(D_PARAM Target). The source type is
@@ -1622,6 +1623,20 @@ unittest
assert(n == 255);
}
+unittest // Unittest for bug 8729
+{
+ assertThrown!ConvException(to!byte (" 1"));
+ assertThrown!ConvException(to!byte (" 1", 4));
+ assertThrown!ConvException(to!int (" 1"));
+ assertThrown!ConvException(to!int (" 1", 4));
+ assertThrown!ConvException(to!long (" 1"));
+ assertThrown!ConvException(to!long (" 1", 4));
+ assertThrown!ConvException(to!double(" 1"));
+ assertThrown!ConvException(to!bool (" false"));
+ assertThrown!ConvException(to!int (" "));
+ assertThrown!ConvException(to!int (""));
+}
+
/***************************************************************
Rounded conversion from floating point to integral.
@@ -1711,8 +1726,6 @@ Target parse(Target, Source)(ref Source s)
else
{
// Larger than int types
- if (s.empty)
- goto Lerr;
@monarchdodra Collaborator

This test was not needed, as the code naturally handles empty. No point in paying every time for a case which should rarilly happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
static if (Target.min < 0)
int sign = 0;
@@ -2131,13 +2144,8 @@ Target parse(Target, Source)(ref Source p)
return new ConvException(text(msg, " for input \"", p, "\"."), fn, ln);
}
- for (;;)
- {
- enforce(!p.empty, bailOut());
- if (!std.uni.isWhite(p.front))
- break;
- p.popFront();
- }
+ enforce(!p.empty && !std.uni.isWhite(p.front), bailOut());
+
char sign = 0; /* indicating + */
switch (p.front)
{
@@ -2595,6 +2603,7 @@ Target parse(Target, Source)(ref Source s)
if (isSomeString!Source && !is(Source == enum) &&
staticIndexOf!(Unqual!Target, dchar, Unqual!(ElementEncodingType!Source)) >= 0)
{
+ if(s.empty) convError!(Source, Target)(s);
static if (is(Unqual!Target == dchar))
{
Target result = s.front;
@@ -2631,6 +2640,7 @@ Target parse(Target, Source)(ref Source s)
if (!isSomeString!Source && isInputRange!Source && isSomeChar!(ElementType!Source) &&
isSomeChar!Target && Target.sizeof >= ElementType!Source.sizeof && !is(Target == enum))
{
+ if(s.empty) convError!(Source, Target)(s);
Target result = s.front;
s.popFront();
return result;
@@ -2651,7 +2661,7 @@ Target parse(Target, Source)(ref Source s)
s = s[5 .. $];
return false;
}
- parseError("bool should be case-insensive 'true' or 'false'");
+ parseError("bool should be case-insensitive 'true' or 'false'");
assert(0);
}
@@ -2694,7 +2704,7 @@ Target parse(Target, Source)(ref Source s)
s = s[4 .. $];
return null;
}
- parseError("null should be case-insensive 'null'");
+ parseError("null should be case-insensitive 'null'");
assert(0);
}
@@ -2719,7 +2729,25 @@ unittest
private void skipWS(R)(ref R r)
{
- skipAll(r, ' ', '\n', '\t', '\r');
+ static if(isSomeString!R)
+ {
+ //Implementation inspired from stripLeft.
+ foreach(i, dchar c; r)
+ {
+ if(!std.ascii.isWhite(c))
+ {
+ r = r[i .. $];
+ return;
+ }
+ }
+ r = r[0 .. 0]; //Empty string with correct type.
+ return;
+ }
+ else
+ {
+ for ( ; !r.empty && std.ascii.isWhite(r.front) ; r.popFront())
+ { }
+ }
}
@monarchdodra Collaborator

Arguably more efficient in all cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
/**
@@ -2735,6 +2763,7 @@ Target parse(Target, Source)(ref Source s, dchar lbracket = '[', dchar rbracket
parseCheck!s(lbracket);
skipWS(s);
+ if(s.empty) convError!(Source, Target)(s);
if (s.front == rbracket)
{
s.popFront();
@@ -2744,6 +2773,7 @@ Target parse(Target, Source)(ref Source s, dchar lbracket = '[', dchar rbracket
{
result ~= parseElement!(ElementType!Target)(s);
skipWS(s);
+ if(s.empty) convError!(Source, Target)(s);
if (s.front != comma)
break;
}
@@ -2788,6 +2818,14 @@ unittest
assert(a2 == ["aaa", "bbb", "ccc"]);
}
+unittest
+{
+ //Check proper failure
+ auto ss = "[ 1 , 2 , 3 ]";
+ foreach(i ; 0..ss.length-1)
+ assertThrown!ConvException(parse!(int[])(ss[0 .. i]));
+}
@monarchdodra Collaborator

!Important test!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
/// ditto
Target parse(Target, Source)(ref Source s, dchar lbracket = '[', dchar rbracket = ']', dchar comma = ',')
if (isSomeString!Source && !is(Source == enum) &&
@@ -2797,6 +2835,7 @@ Target parse(Target, Source)(ref Source s, dchar lbracket = '[', dchar rbracket
parseCheck!s(lbracket);
skipWS(s);
+ if(s.empty) convError!(Source, Target)(s);
if (s.front == rbracket)
{
static if (result.length != 0)
@@ -2813,6 +2852,7 @@ Target parse(Target, Source)(ref Source s, dchar lbracket = '[', dchar rbracket
goto Lmanyerr;
result[i++] = parseElement!(ElementType!Target)(s);
skipWS(s);
+ if(s.empty) convError!(Source, Target)(s);
if (s.front != comma)
{
if (i != result.length)
@@ -2866,6 +2906,7 @@ Target parse(Target, Source)(ref Source s, dchar lbracket = '[', dchar rbracket
parseCheck!s(lbracket);
skipWS(s);
+ if(s.empty) convError!(Source, Target)(s);
if (s.front == rbracket)
{
s.popFront();
@@ -2967,10 +3008,12 @@ Target parseElement(Target, Source)(ref Source s)
auto result = appender!Target();
// parse array of chars
+ if(s.empty) convError!(Source, Target)(s);
if (s.front == '[')
return parse!Target(s);
parseCheck!s('\"');
+ if(s.empty) convError!(Source, Target)(s);
if (s.front == '\"')
{
s.popFront();
@@ -3005,6 +3048,7 @@ Target parseElement(Target, Source)(ref Source s)
Target c;
parseCheck!s('\'');
+ if(s.empty) convError!(Source, Target)(s);
if (s.front != '\\')
{
c = s.front;
Something went wrong with that request. Please try again.