search

dlang-community / std_data_json

Phobos candidate JSON implementation.
25 stars 13 forks source link

Remove pointless `@disable this(this)`. #53

Closed FeepingCreature closed 4 years ago

FeepingCreature commented 4 years ago

Why is this there? I don't see any reason to make array ranges a pure input range. It doesn't have save either, not that anyone uses that.

Far as I can tell, VR supports repeated iteration just fine.

s-ludwig commented 4 years ago

Not sure why, but it may have been to make it harder to misuse (harder to illegally escape the by-ref input range or maybe to violate the non-forward property). Since it doesn't really achieve that in a satisfying way, I don't think it's worth it, though.

The behavior for repeated iteration is generally undefined here, it may work in some cases, but generally, JSONParserRange is not usable as a forward range as it stands.

FeepingCreature commented 4 years ago

Sure, but this specific range is though, right?

Which JSONParserRange isn't usable as a forward range? (And why? Mutable data in a closure?)

s-ludwig commented 4 years ago

It accesses front of the nodes pointer, which can be changed by popFront from any VR or ARR copy that references the same pointer.

To make this work correctly, the whole range chain would have to define a save method, with JSONParserRange in particular duplicating its _containerStack member and calling save on the underlying range.

FeepingCreature commented 4 years ago

Oh! I see. Damn. I misinterpreted that popFront hierarchy...

Well, let me see if I can work out how to make that work.

FeepingCreature commented 4 years ago

Ah, it's just because we need to modify our parameter while we iterate, I finally get it. I just removed that function entirely from my local copy, and used range.readArray({}) instead. Problem solved :)