On Mon, Aug 26, 2013 at 02:30:09PM -0700, H. S. Teoh wrote:
> On Sun, Aug 25, 2013 at 12:18:27AM +0200, Tobias Pankrath wrote:
> > On Saturday, 24 August 2013 at 20:11:14 UTC, H. S. Teoh wrote:
> > >How would you use RAII to solve this problem? If I have a class:
> > >
> > >	class C {
> > >		Resource1 res1;
> > >		Resource2 res2;
> > >		Resource3 res3;
> > >		this() {
> > >			...
> > >		}
> > >	}
> > >
> > >How would you write the ctor with RAII such that if it successfully inits res1 and res2, but throws before it inits res3, then only res1 and res2 will be cleaned up?
> > 
> > Like someone else already proposed: Using a wrapper type W that releases the resources in it's destructor. W.init wouldn't release anything. So by definition (if I recall the rules correctly) every new instance of C would have res3 = Resource3.init prior to it's constructor called.
> > 
> > Now make sure that a) res3's destructor gets called (check) b)
> > res3's destructor may be called on Resource3.init. That would be a
> > new rule similar to 'no internal aliasing' and c) that every
> > constructor of C either sets res3 to a destructable value or does
> > not touch it at all ('transactional programming').
> [...]
> 
> But don't you still need to manually cleanup in the case of ctor failure? AFAIK, the dtor is not invoked on the partially-constructed object if the ctor throws. So you'd still have to use scope(failure) to manually release the resource.
> 
> To prove my point, here is some sample code that (tries to) use RAII to
> cleanup:
> 
> 	import std.stdio;
> 
> 	struct Resource {
> 		int x = 0;
> 		this(int id) {
> 			x = id;
> 			writefln("Acquired resource %d", x);
> 		}
> 		~this() {
> 			if (x == 0)
> 				writefln("Empty resource, no cleanup");
> 			else
> 				writefln("Cleanup resource %d", x);
> 		}
> 	}
> 
> 	struct S {
> 		Resource res1;
> 		Resource res2;
> 		Resource res3;
> 
> 		this(int) {
> 			res1 = Resource(1);
> 			res2 = Resource(2);
> 			assert(res1.x == 1);
> 			assert(res2.x == 2);
> 
> 			throw new Exception("ctor failed!");
> 			res3 = Resource(3);	// not reached
> 			assert(res3.x == 3);	// not reached
> 		}
> 	}
> 
> 	void main() {
> 		try {
> 			auto s = S(123);
> 		} catch(Exception e) {
> 			writeln(e.msg);
> 		}
> 	}
> 
> Here is the program output:
> 
> 	Acquired resource 1
> 	Empty resource, no cleanup
> 	Acquired resource 2
> 	Empty resource, no cleanup
> 	ctor failed!
> 
> As you can see, the two resources acquired in the partially-constructed object did NOT get cleaned up. So, RAII doesn't work in this case.
> 
> The two interspersed cleanups were presumably cause by Resource.init being destructed when res1 and res2 were assigned to. But after being assigned to, res1 and res2's dtors were NOT invoked.
> 
> So I think my scope(this) idea has some merit, since it will correctly
> handle this case. :)
[...]

P.S. If you comment out the throw line, you will see that the resources are released correctly. So, RAII lets us handle automatic cleanup without needing an explicit dtor, but it (currently) does NOT correctly handle cleanup for the partially-constructed object case.

To solve this problem, basically the compiler will have to keep track of which fields have been initialized already, and insert scope(failure) statements for them in the ctor. But once you have that, you have basically already implemented scope(this), so might as well go all the way and give it a nice syntax.


T

-- 
Let's not fight disease by killing the patient. -- Sean 'Shaleh' Perry

On Monday, 26 August 2013 at 21:31:40 UTC, H. S. Teoh wrote:


> But don't you still need to manually cleanup in the case of ctor
> failure? AFAIK, the dtor is not invoked on the partially-constructed
> object if the ctor throws.

I didn't knew this. You are right, that this brakes my RAII scheme and something to work around this would be helpful. That's actually quite the pitfall.

27-Aug-2013 01:30, H. S. Teoh пишет:
> On Sun, Aug 25, 2013 at 12:18:27AM +0200, Tobias Pankrath wrote:
>> On Saturday, 24 August 2013 at 20:11:14 UTC, H. S. Teoh wrote:
>>> How would you use RAII to solve this problem? If I have a class:
>>>
>>> 	class C {
>>> 		Resource1 res1;
>>> 		Resource2 res2;
>>> 		Resource3 res3;
>>> 		this() {
>>> 			...
>>> 		}
>>> 	}
>>>
>>> How would you write the ctor with RAII such that if it successfully
>>> inits res1 and res2, but throws before it inits res3, then only res1
>>> and res2 will be cleaned up?
>>
>> Like someone else already proposed: Using a wrapper type W that
>> releases the resources in it's destructor. W.init wouldn't release
>> anything. So by definition (if I recall the rules correctly) every
>> new instance of C would have res3 = Resource3.init prior to it's
>> constructor called.
>>
>> Now make sure that a) res3's destructor gets called (check) b)
>> res3's destructor may be called on Resource3.init. That would be a
>> new rule similar to 'no internal aliasing' and c) that every
>> constructor of C either sets res3 to a destructable value or does
>> not touch it at all ('transactional programming').
> [...]
>
> But don't you still need to manually cleanup in the case of ctor
> failure? AFAIK, the dtor is not invoked on the partially-constructed
> object if the ctor throws. So you'd still have to use scope(failure) to
> manually release the resource.
>
> To prove my point, here is some sample code that (tries to) use RAII to
> cleanup:
>
> 	import std.stdio;
> 	
> 	struct Resource {
> 		int x = 0;
> 		this(int id) {
> 			x = id;
> 			writefln("Acquired resource %d", x);
> 		}
> 		~this() {
> 			if (x == 0)
> 				writefln("Empty resource, no cleanup");
> 			else
> 				writefln("Cleanup resource %d", x);
> 		}
> 	}
> 	
> 	struct S {
> 		Resource res1;
> 		Resource res2;
> 		Resource res3;
> 	
> 		this(int) {
> 			res1 = Resource(1);
> 			res2 = Resource(2);
> 			assert(res1.x == 1);
> 			assert(res2.x == 2);
> 	
> 			throw new Exception("ctor failed!");
> 			res3 = Resource(3);	// not reached
> 			assert(res3.x == 3);	// not reached
> 		}
> 	}
> 	
> 	void main() {
> 		try {
> 			auto s = S(123);
> 		} catch(Exception e) {
> 			writeln(e.msg);
> 		}
> 	}
>
> Here is the program output:
>
> 	Acquired resource 1
> 	Empty resource, no cleanup
> 	Acquired resource 2
> 	Empty resource, no cleanup
> 	ctor failed!
>
> As you can see, the two resources acquired in the partially-constructed
> object did NOT get cleaned up. So, RAII doesn't work in this case.

Fixed?

> 			auto r1 = Resource(1);
> 			auto r2 = Resource(2);
> 			assert(res1.x == 1);
> 			assert(res2.x == 2);
> 	
> 			throw new Exception("ctor failed!");
> 			auto r3 = Resource(3);	// not reached
> 			assert(res3.x == 3);	// not reached

			res1 = move(r1);
			res2 = move(r2);
			res3 = move(r3);


Exception-safe code after all has to be exception safe.
Unlike in C++ we have no explicit initialization of members (and strict order of this) hence no automatic partial cleanup.

-- 
Dmitry Olshansky

On Tue, Aug 27, 2013 at 04:01:57PM +0400, Dmitry Olshansky wrote:
> 27-Aug-2013 01:30, H. S. Teoh пишет:
> >On Sun, Aug 25, 2013 at 12:18:27AM +0200, Tobias Pankrath wrote:
> >>On Saturday, 24 August 2013 at 20:11:14 UTC, H. S. Teoh wrote:
> >>>How would you use RAII to solve this problem? If I have a class:
> >>>
> >>>	class C {
> >>>		Resource1 res1;
> >>>		Resource2 res2;
> >>>		Resource3 res3;
> >>>		this() {
> >>>			...
> >>>		}
> >>>	}
> >>>
> >>>How would you write the ctor with RAII such that if it successfully inits res1 and res2, but throws before it inits res3, then only res1 and res2 will be cleaned up?
> >>
> >>Like someone else already proposed: Using a wrapper type W that releases the resources in it's destructor. W.init wouldn't release anything. So by definition (if I recall the rules correctly) every new instance of C would have res3 = Resource3.init prior to it's constructor called.
> >>
> >>Now make sure that a) res3's destructor gets called (check) b)
> >>res3's destructor may be called on Resource3.init. That would be a
> >>new rule similar to 'no internal aliasing' and c) that every
> >>constructor of C either sets res3 to a destructable value or does
> >>not touch it at all ('transactional programming').
> >[...]
> >
> >But don't you still need to manually cleanup in the case of ctor failure? AFAIK, the dtor is not invoked on the partially-constructed object if the ctor throws. So you'd still have to use scope(failure) to manually release the resource.
> >
> >To prove my point, here is some sample code that (tries to) use RAII to
> >cleanup:
> >
> >	import std.stdio;
> >
> >	struct Resource {
> >		int x = 0;
> >		this(int id) {
> >			x = id;
> >			writefln("Acquired resource %d", x);
> >		}
> >		~this() {
> >			if (x == 0)
> >				writefln("Empty resource, no cleanup");
> >			else
> >				writefln("Cleanup resource %d", x);
> >		}
> >	}
> >
> >	struct S {
> >		Resource res1;
> >		Resource res2;
> >		Resource res3;
> >
> >		this(int) {
> >			res1 = Resource(1);
> >			res2 = Resource(2);
> >			assert(res1.x == 1);
> >			assert(res2.x == 2);
> >
> >			throw new Exception("ctor failed!");
> >			res3 = Resource(3);	// not reached
> >			assert(res3.x == 3);	// not reached
> >		}
> >	}
> >
> >	void main() {
> >		try {
> >			auto s = S(123);
> >		} catch(Exception e) {
> >			writeln(e.msg);
> >		}
> >	}
> >
> >Here is the program output:
> >
> >	Acquired resource 1
> >	Empty resource, no cleanup
> >	Acquired resource 2
> >	Empty resource, no cleanup
> >	ctor failed!
> >
> >As you can see, the two resources acquired in the partially-constructed object did NOT get cleaned up. So, RAII doesn't work in this case.
> 
> Fixed?
> 
> > 			auto r1 = Resource(1);
> > 			auto r2 = Resource(2);
> > 			assert(res1.x == 1);
> > 			assert(res2.x == 2);
> > 
> > 			throw new Exception("ctor failed!");
> > 			auto r3 = Resource(3);	// not reached
> > 			assert(res3.x == 3);	// not reached
> 
> 			res1 = move(r1);
> 			res2 = move(r2);
> 			res3 = move(r3);
> 
> 
> Exception-safe code after all has to be exception safe.
> Unlike in C++ we have no explicit initialization of members (and
> strict order of this) hence no automatic partial cleanup.
[...]

What if move(r2) throws? Then res1 won't get cleaned up, because r1 has already been nulled by the move.


T

-- 
Without outlines, life would be pointless.

On Tuesday, 27 August 2013 at 14:26:53 UTC, H. S. Teoh wrote:
> What if move(r2) throws? Then res1 won't get cleaned up, because r1 has
> already been nulled by the move.
>

I don't think move can throw.

On Tue, Aug 27, 2013 at 05:46:40PM +0200, deadalnix wrote:
> On Tuesday, 27 August 2013 at 14:26:53 UTC, H. S. Teoh wrote:
> >What if move(r2) throws? Then res1 won't get cleaned up, because r1 has already been nulled by the move.
> >
> 
> I don't think move can throw.

Well, it's not marked nothrow, so I wouldn't count on that.

Also, the fact that move() uses memcpy is a bit worrying; Adam Ruppe & myself ran into a nasty bug involving closures over struct members when the struct may get moved upon return from a function. For example:

	struct S {
		int id;
		void delegate()[] cleanups;

		this() {
			id = acquireResource();
			cleanups ~= {
				// N.B.: closure over this.id
				releaseResource(id);
			};
		}
	}

	S makeS() {
		// Problem: S.cleanups[0] is a closure over the struct
		// instance on this function's stack, but once S is
		// returned, it gets memcpy'd into the caller's stack
		// frame. This invalidates the delegate's context
		// pointer.
		return S(1);
	}

	void main() {
		auto s = makeS();
		// Problem: s.cleanups[0] now has an invalid context
		// pointer. If the stack is reused after this point, the
		// dtor of s will get a garbage value for s.id.
	}

Using move() to move a resource from a local variable into a member looks like it might be vulnerable to this bug as well -- if the resource has closures over member variables it might trigger this problem.


T

-- 
To provoke is to call someone stupid; to argue is to call each other stupid.

27-Aug-2013 18:25, H. S. Teoh пишет:
> On Tue, Aug 27, 2013 at 04:01:57PM +0400, Dmitry Olshansky wrote:

>
> What if move(r2) throws? Then res1 won't get cleaned up, because r1 has
> already been nulled by the move.
>

Then something is terribly wrong :)

Rule #1 of move should be is that it doesn't throw.

It just blits the damn data. Even if this is currently broken..
At the very least 2-arg version certainly can avoid throw even if T has a bogus destructor that goes bottom up on T.init.

BTW same with swap and at least in C++ that's the only way to avoid exceptions creeping up from nowhere.


-- 
Dmitry Olshansky

On Wed, Aug 28, 2013 at 12:24:43AM +0400, Dmitry Olshansky wrote:
> 27-Aug-2013 18:25, H. S. Teoh пишет:
> >On Tue, Aug 27, 2013 at 04:01:57PM +0400, Dmitry Olshansky wrote:
> 
> >
> >What if move(r2) throws? Then res1 won't get cleaned up, because r1 has already been nulled by the move.
> >
> 
> Then something is terribly wrong :)
> 
> Rule #1 of move should be is that it doesn't throw.

std.algorithm.move is not nothrow. I tried to make it nothrow, and the compiler told me:

std/algorithm.d(1604): Error: 'object.TypeInfo.destroy' is not nothrow
std/algorithm.d(1596): Error: function 'std.algorithm.move!(DirIteratorImpl).move' is nothrow yet may throw
std/typecons.d(3522): Error: template instance std.algorithm.move!(DirIteratorImpl) error instantiating
std/file.d(2526):        instantiated from here: RefCounted!(DirIteratorImpl, cast(RefCountedAutoInitialize)0)
std/algorithm.d(1604): Error: 'object.TypeInfo.destroy' is not nothrow
std/algorithm.d(1596): Error: function 'std.algorithm.move!(Impl).move' is nothrow yet may throw
std/typecons.d(3522): Error: template instance std.algorithm.move!(Impl) error instantiating
std/net/curl.d(2040):        instantiated from here: RefCounted!(Impl)
std/algorithm.d(1604): Error: 'object.TypeInfo.destroy' is not nothrow
std/algorithm.d(1596): Error: function 'std.algorithm.move!(Impl).move' is nothrow yet may throw
std/typecons.d(3522): Error: template instance std.algorithm.move!(Impl) error instantiating
std/net/curl.d(2747):        instantiated from here: RefCounted!(Impl)
std/algorithm.d(1604): Error: 'object.TypeInfo.destroy' is not nothrow
std/algorithm.d(1596): Error: function 'std.algorithm.move!(Impl).move' is nothrow yet may throw
std/typecons.d(3522): Error: template instance std.algorithm.move!(Impl) error instantiating
std/net/curl.d(3065):        instantiated from here: RefCounted!(Impl)

Isn't that scary? This means that the case I presented above is not hypothetical -- if destroy throws, then you're screwed.

(And yes it's a very very rare case... but would you want to find out the hard way when a problem that triggers a throw in destroy slips past QA testing 'cos it's so rare, and shows up in a customer's environment where it may take hours or days or even months to debug?)

Basically, the only way to be 100% sure is to use scope(this), or the
manual equivalent thereof (see below).


> It just blits the damn data. Even if this is currently broken..

It also calls destroy, which is not nothrow. :)


> At the very least 2-arg version certainly can avoid throw even if T has a bogus destructor that goes bottom up on T.init.
> 
> BTW same with swap and at least in C++ that's the only way to avoid exceptions creeping up from nowhere.
[...]

Well this is kinda getting away from the main point, which is that scope(this) allows us to hide all these dirty implementation details under a compiler-implemented solution that makes sure things are always done right. This is part of the reason I wrote DIP44: although it *is* possible to manually write code that behaves correctly, it's pretty tricky, and I doubt many people even realize the potential pitfalls. Ideally, straightforward D code should also be correct by default.

Anyway, none of this move/nothrow nonsense is needed if we write things this way:

	struct S {
		void delegate()[] __cleanups;
		Resource res1;
		Resource res2;

		this() {
			scope(failure) __cleanup();
			res1 = acquireResource();
			__cleanups ~= { res1.release(); };

			res2 = acquireResource();
			__cleanups ~= { res2.release(); };
		}

		~this() { __cleanup(); }

		private void __cleanup() {
			foreach_reverse (f; __cleanups)
				f();
		}
	}

Which is basically what DIP44 is proposing under a nicer syntax.


T

-- 
"640K ought to be enough" -- Bill G., 1984. "The Internet is not a primary goal for PC usage" -- Bill G., 1995. "Linux has no impact on Microsoft's strategy" -- Bill G., 1999.

28-Aug-2013 00:41, H. S. Teoh пишет:
> On Wed, Aug 28, 2013 at 12:24:43AM +0400, Dmitry Olshansky wrote:
>> 27-Aug-2013 18:25, H. S. Teoh пишет:
>>> On Tue, Aug 27, 2013 at 04:01:57PM +0400, Dmitry Olshansky wrote:
>>
>>>
>>> What if move(r2) throws? Then res1 won't get cleaned up, because r1 has
>>> already been nulled by the move.
>>>
>>
>> Then something is terribly wrong :)
>>
>> Rule #1 of move should be is that it doesn't throw.
>
> std.algorithm.move is not nothrow. I tried to make it nothrow, and the
> compiler told me:
>
> std/algorithm.d(1604): Error: 'object.TypeInfo.destroy' is not nothrow
> std/algorithm.d(1596): Error: function 'std.algorithm.move!(DirIteratorImpl).move' is nothrow yet may throw
> std/typecons.d(3522): Error: template instance std.algorithm.move!(DirIteratorImpl) error instantiating
> std/file.d(2526):        instantiated from here: RefCounted!(DirIteratorImpl, cast(RefCountedAutoInitialize)0)
> std/algorithm.d(1604): Error: 'object.TypeInfo.destroy' is not nothrow
> std/algorithm.d(1596): Error: function 'std.algorithm.move!(Impl).move' is nothrow yet may throw
> std/typecons.d(3522): Error: template instance std.algorithm.move!(Impl) error instantiating
> std/net/curl.d(2040):        instantiated from here: RefCounted!(Impl)
> std/algorithm.d(1604): Error: 'object.TypeInfo.destroy' is not nothrow
> std/algorithm.d(1596): Error: function 'std.algorithm.move!(Impl).move' is nothrow yet may throw
> std/typecons.d(3522): Error: template instance std.algorithm.move!(Impl) error instantiating
> std/net/curl.d(2747):        instantiated from here: RefCounted!(Impl)
> std/algorithm.d(1604): Error: 'object.TypeInfo.destroy' is not nothrow
> std/algorithm.d(1596): Error: function 'std.algorithm.move!(Impl).move' is nothrow yet may throw
> std/typecons.d(3522): Error: template instance std.algorithm.move!(Impl) error instantiating
> std/net/curl.d(3065):        instantiated from here: RefCounted!(Impl)
>
> Isn't that scary? This means that the case I presented above is not
> hypothetical -- if destroy throws, then you're screwed.

Sorry but this is hypocritical. Obviously nothrow attribute wouldn''t work as the only thing we must ensure is that T.init is safely destroyable.

>
> (And yes it's a very very rare case... but would you want to find out
> the hard way when a problem that triggers a throw in destroy slips past
> QA testing 'cos it's so rare, and shows up in a customer's environment
> where it may take hours or days or even months to debug?)

I'm not sold. Bugs are bugs in any case.

>
> Basically, the only way to be 100% sure is to use scope(this),

That yet has to be implemented and is full of interesting interplay with other features. No thanks.
>  or the
> manual equivalent thereof (see below).

That is flawed or use a different code practice.

>
>> It just blits the damn data. Even if this is currently broken..
>
> It also calls destroy, which is not nothrow. :)

Oh my. And how many things are by @safe but can't be.
The only assertion missing is that destroy of T.init can't throw.
It is btw a common bug...
https://github.com/D-Programming-Language/phobos/pull/1523

>
>> At the very least 2-arg version certainly can avoid throw even if T
>> has a bogus destructor that goes bottom up on T.init.
>>
>> BTW same with swap and at least in C++ that's the only way to avoid
>> exceptions creeping up from nowhere.
> [...]
>
> Well this is kinda getting away from the main point, which is that
> scope(this) allows us to hide all these dirty implementation details
> under a compiler-implemented solution that makes sure things are always
> done right. This is part of the reason I wrote DIP44: although it *is*
> possible to manually write code that behaves correctly, it's pretty
> tricky, and I doubt many people even realize the potential pitfalls.
> Ideally, straightforward D code should also be correct by default.

It's always seems easy to hide away an elephant in the compiler. Please let's stop this tendency.
Anyhow if we are to reach there a better solution that occurred to me would be to make compiler call destructors on "cooked" fields as it already keeps track of these anyway. It rides on the same mechanism as initializing immutables in constructor. Then original code with 3 RAII wrappers works out of the box and job is done.

> Anyway, none of this move/nothrow nonsense is needed if we write things
> this way:

Wrong. Make that 3 resources and then what if res2.release throws? - you never get to the first one. Or simply res2.release throws in destructor even with 2 of them, chain of cleanups doesn't work. You have to have primitives that don't throw at some level of this or continue wrapping in try/finally:)

	struct S {
		void delegate()[] __cleanups;
		Resource res1;
		Resource res2;
		Resource res3;

		this() {
			scope(failure) __cleanup();
			res1 = acquireResource();
			__cleanups ~= { res1.release(); };

			res2 = acquireResource();
			__cleanups ~= { res2.release(); };

			res3 = acquireResource();
			__cleanups ~= { res3.release(); };
		}

		~this() { __cleanup(); }

		private void __cleanup() {
			foreach_reverse (f; __cleanups)
				f();
		}
	}


-- 
Dmitry Olshansky

On Wed, Aug 28, 2013 at 01:02:27AM +0400, Dmitry Olshansky wrote:
> 28-Aug-2013 00:41, H. S. Teoh пишет:
> >On Wed, Aug 28, 2013 at 12:24:43AM +0400, Dmitry Olshansky wrote:
> >>27-Aug-2013 18:25, H. S. Teoh пишет:
> >>>On Tue, Aug 27, 2013 at 04:01:57PM +0400, Dmitry Olshansky wrote:
> >>
> >>>
> >>>What if move(r2) throws? Then res1 won't get cleaned up, because r1 has already been nulled by the move.
> >>>
> >>
> >>Then something is terribly wrong :)
> >>
> >>Rule #1 of move should be is that it doesn't throw.
> >
> >std.algorithm.move is not nothrow. I tried to make it nothrow, and the compiler told me:
> >
> >std/algorithm.d(1604): Error: 'object.TypeInfo.destroy' is not nothrow
> >std/algorithm.d(1596): Error: function 'std.algorithm.move!(DirIteratorImpl).move' is nothrow yet may throw
> >std/typecons.d(3522): Error: template instance std.algorithm.move!(DirIteratorImpl) error instantiating
> >std/file.d(2526):        instantiated from here: RefCounted!(DirIteratorImpl, cast(RefCountedAutoInitialize)0)
> >std/algorithm.d(1604): Error: 'object.TypeInfo.destroy' is not nothrow
> >std/algorithm.d(1596): Error: function 'std.algorithm.move!(Impl).move' is nothrow yet may throw
> >std/typecons.d(3522): Error: template instance std.algorithm.move!(Impl) error instantiating
> >std/net/curl.d(2040):        instantiated from here: RefCounted!(Impl)
> >std/algorithm.d(1604): Error: 'object.TypeInfo.destroy' is not nothrow
> >std/algorithm.d(1596): Error: function 'std.algorithm.move!(Impl).move' is nothrow yet may throw
> >std/typecons.d(3522): Error: template instance std.algorithm.move!(Impl) error instantiating
> >std/net/curl.d(2747):        instantiated from here: RefCounted!(Impl)
> >std/algorithm.d(1604): Error: 'object.TypeInfo.destroy' is not nothrow
> >std/algorithm.d(1596): Error: function 'std.algorithm.move!(Impl).move' is nothrow yet may throw
> >std/typecons.d(3522): Error: template instance std.algorithm.move!(Impl) error instantiating
> >std/net/curl.d(3065):        instantiated from here: RefCounted!(Impl)
> >
> >Isn't that scary? This means that the case I presented above is not hypothetical -- if destroy throws, then you're screwed.
> 
> Sorry but this is hypocritical. Obviously nothrow attribute wouldn''t work as the only thing we must ensure is that T.init is safely destroyable.

I'm not sure I understand you. Since the compiler tells me that std.algorithm.move can't be made nothrow, I, from a user's POV, have no choice but to believe that there are *some* circumstances in which it *will* throw.


> >(And yes it's a very very rare case... but would you want to find out the hard way when a problem that triggers a throw in destroy slips past QA testing 'cos it's so rare, and shows up in a customer's environment where it may take hours or days or even months to debug?)
> 
> I'm not sold. Bugs are bugs in any case.

Yes, but if you could do something today to prevent bugs in the future, why not?


> >Basically, the only way to be 100% sure is to use scope(this),
> 
> That yet has to be implemented and is full of interesting interplay with other features. No thanks.
>
> > or the manual equivalent thereof (see below).
> 
> That is flawed or use a different code practice.

How is it flawed?


> >>It just blits the damn data. Even if this is currently broken..
> >
> >It also calls destroy, which is not nothrow. :)
> 
> Oh my. And how many things are by @safe but can't be.
> The only assertion missing is that destroy of T.init can't throw.
> It is btw a common bug...
> https://github.com/D-Programming-Language/phobos/pull/1523

Yes, this is a common bug. So it's a real possibility that move() may throw. And now we know that if File's were involved, it could actually happen.  ;-)

I'd argue that, independently of DIP44, destroy() should be marked
nothrow:

	void destroy(T)(ref T t) nothrow {
		...
		try {
			callDtor(t);
			// or whatever the syntax is, I don't remember
			// now.
		} catch(Error e) {
			// If we get here, we're royally screwed anyway,
			// so might as well give up.
			assert(0);
		}
	}

I mean, if an object is going out of scope or being GC'd and the dtor is invoked, and the dtor throws an exception, then what you are gonna do? Just catch the exception and try to destroy the object again? You can't, because by the time the stack unwinds the object doesn't exist anymore.

Or, in the case of move(), the code cannot continue because it needs to destroy the previous object but the destruction failed, so what is the catcher going to do? It may not have access to the scope in which the object is defined, so it can't possibly do any real cleanup. And if the object was created in a scope below the catch block, the unwinding code will encounter the dtor exception a second time... in any case, we're completely screwed so there's no point in continuing.

Once move() is nothrow, then I agree you have a point that you can initialize resources as ctor local variables first, then assign them to member fields.

Ultimately, though, in machine code it's not really any different from
putting scope(failure) __cleanup() in the ctor. Just the same solution
implemented in different ways.


> >>At the very least 2-arg version certainly can avoid throw even if T has a bogus destructor that goes bottom up on T.init.
> >>
> >>BTW same with swap and at least in C++ that's the only way to avoid exceptions creeping up from nowhere.
> >[...]
> >
> >Well this is kinda getting away from the main point, which is that scope(this) allows us to hide all these dirty implementation details under a compiler-implemented solution that makes sure things are always done right. This is part of the reason I wrote DIP44: although it *is* possible to manually write code that behaves correctly, it's pretty tricky, and I doubt many people even realize the potential pitfalls. Ideally, straightforward D code should also be correct by default.
> 
> It's always seems easy to hide away an elephant in the compiler.
> Please let's stop this tendency.
> Anyhow if we are to reach there a better solution that occurred to
> me would be to make compiler call destructors on "cooked" fields as
> it already keeps track of these anyway. It rides on the same
> mechanism as initializing immutables in constructor. Then original
> code with 3 RAII wrappers works out of the box and job is done.

Does the compiler keep track of what has been initialized? If it does, then I agree that it should call dtors on "cooked" fields on ctor failure. That would eliminate a large part of the reason for DIP44.

As it stands, though, partially-initialized objects are just left as-is,
with no attempt to cleanup (as I've shown in a previous post). The only
way around this currently is to manually keep track of what has been
initialized and use scope(failure) (or equivalent) to cleanup.


> > Anyway, none of this move/nothrow nonsense is needed if we write things this way:
> 
> Wrong. Make that 3 resources and then what if res2.release throws? - you never get to the first one. Or simply res2.release throws in destructor even with 2 of them, chain of cleanups doesn't work. You have to have primitives that don't throw at some level of this or continue wrapping in try/finally:)

Well, OK, so make the definition of __cleanups nothrow:

	alias CleanupFunc = void delegate() nothrow;
	CleanupFunc[] __cleanups;

Then the compiler can statically reject any cleanup code that may throw. Currently, I can't say the same for std.algorithm.move().


T

-- 
Маленькие детки - маленькие бедки.