I vote to accept the library into Boost. * What is your evaluation of the design? * What is your evaluation of the implementation? The design and implementation is clear and straightforward. Of course we'd like to have such a library with better syntax, but current C++ doesn't allow to make it better. * What is your evaluation of the documentation? Clear and understandable. * What is your evaluation of the potential usefulness of the library? I believe the library is very useful, I will use it in my work. It helps to avoid writing complex rollback code clean and in-place. Another nice feature comparing to using lambdas is the ability to place breakpoints inside the rollback code. Without ScopeExit you can only achieve this by using manually-written scope guard class from scratch * Did you try to use the library? With what compiler? Did you have any problems? Yes, played with the examples from documentation, GCC 3.4, no problems noted. * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? Compiled, played with examples, looked into the implementation. * Are you knowledgeable about the problem domain? I'm sure everyone is :) * Do you think the library should be accepted as a Boost library? Yes. A couple of general comments after reading other reviews. 1. The fact that the library takes all arguments by reference looks unavoidable. If it allowed passing by value (i.e. making copies) - the copy constructors can throw, and this exception will be thrown after you made the change to your container/whatever you'd like to rollback, and before you setup the rollback action. So you can't guarantee that your rollback action will be executed. So, in case you need a copy - it's much safer to create this copy before making the change to your container/whatever. It will also express your intention more clearly. 2. Of course it would be great to have a full-featured scope guard library in Boost, but its absense is not an excuse to reject ScopeExit, otherwise we need to reject scoped_ptr/scoped_array etc, as they are forms of scope guard. In general, I believe we can't reject a library A just because Boost misses a library B. Thanks, Maxim
Hello Maxim, Wednesday, August 22, 2007, 9:54:59 PM, you wrote:
A couple of general comments after reading other reviews.
1. The fact that the library takes all arguments by reference looks unavoidable. If it allowed passing by value (i.e. making copies) - the copy constructors can throw, and this exception will be thrown after you made the change to your container/whatever you'd like to rollback, and before you setup the rollback action. So you can't guarantee that your rollback action will be executed.
So, in case you need a copy - it's much safer to create this copy before making the change to your container/whatever. It will also express your intention more clearly.
Generally speaking, you are right here. But first, there are many objects that don't throw on copying. And second, it's not always possible to make a copy beforehand. See one of my previous posts for an example (inserting into container yelds an iterator that is to be used in the scope-exit block). So I think the best way is to leave the decision up to user. -- Best regards, Andrey mailto:andysem@mail.ru
AMDG Andrey Semashev <andysem <at> mail.ru> writes:
Generally speaking, you are right here. But first, there are many objects that don't throw on copying. And second, it's not always possible to make a copy beforehand. See one of my previous posts for an example (inserting into container yelds an iterator that is to be used in the scope-exit block). So I think the best way is to leave the decision up to user.
I don't understand. Why can't you make a copy right before the scope exit block? I think that for ScopeExit, always passing by reference is correct. At the very least it should be the default. In Christ, Steven Watanabe
Hello Steven, Wednesday, August 22, 2007, 11:33:46 PM, you wrote:
AMDG
Andrey Semashev <andysem <at> mail.ru> writes:
Generally speaking, you are right here. But first, there are many objects that don't throw on copying. And second, it's not always possible to make a copy beforehand. See one of my previous posts for an example (inserting into container yelds an iterator that is to be used in the scope-exit block). So I think the best way is to leave the decision up to user.
I don't understand. Why can't you make a copy right before the scope exit block? I think that for ScopeExit, always passing by reference is correct. At the very least it should be the default.
I can. But this is a burden since it should be done for almost all args I ever pass to ScopeExit. And I can easily forget to do so and get subtle errors I pointed out before. So this is by no way should be the default behavior. -- Best regards, Andrey mailto:andysem@mail.ru
Hi Andrey, Andrey Semashev <andysem <at> mail.ru> writes:
I don't understand. Why can't you make a copy right before the scope exit block? I think that for ScopeExit, always passing by reference is correct. At the very least it should be the default.
I can. But this is a burden since it should be done for almost all args I ever pass to ScopeExit. And I can easily forget to do so and get subtle errors I pointed out before. So this is by no way should be the default behavior.
Let's look at your example. std::set< int >::iterator it = my_set.insert(10); BOOST_SCOPE_EXIT((my_set)(it)(commit)) { // Which one will be erased here? // Not necessarily the one we intended to. if (!commit) my_set.erase(it); } BOOST_SCOPE_EXIT_END First of all, even while you state:
the only exception to this is the "commit/dispose" flag. Playing otherwise may lead to subtle errors.
In your case my_set should be also sent be reference, and if it's passed by value, you'll get the mentioned subtle errors (you will delete from the copy of the set, and the original set will be unchanged). So you're just switching from one set of subtle errors to another one. But additionally to this another set of subtle errors you also break strong exception guarantee. But if your copy ctor throws, you're in trouble at the line 1, because a new object is created here, and it's created after the insertion, and it can throw. It means that you will not reach ScopeExit block, i.e. your insertion will not be rolled back. However, there are cases when copy ctor can fail, but assignment (or some other form of initialization/copying) can't (for example, ctor of std::vector can throw, but if you created and reserved enough memory successfully, assignment will not throw as there is no allocation). So you need to create a temporary object before the insertion, and then initialize it in some no-throw fashion. For example, consider the following code: extern vector<int>& mutate(); vector<int> v = mutate(); // 1 BOOST_SCOPE_EXIT((v)(commit)) The mutate() changes something and returns a reference to the changes. Here in the line 1 ctor of v can throw, so you won't reach ScopeExit, and the mutation won't be rolled back. The safe way here is to write vector<int> v; // 1 v.reserve(MAX_MUTATION_NUMBER); // 2 v = mutate(); // 3 BOOST_SCOPE_EXIT((v)(commit)) Here the lines 1 and 2 can throw, and mutate() itself can also throw, but not the assignment. Therefore now, given that mutate() executed succesfully, you have guarantee that the ScopeExit block will be executed. To conclude, I don't think it worth to make default an option that breaks strong exception guarantee, if there is one that doesn't. Thanks, Maxim
Hello Maxim, Thursday, August 23, 2007, 10:17:18 AM, you wrote:
Hi Andrey,
Andrey Semashev <andysem <at> mail.ru> writes:
I can. But this is a burden since it should be done for almost all args I ever pass to ScopeExit. And I can easily forget to do so and get subtle errors I pointed out before. So this is by no way should be the default behavior.
Let's look at your example.
[snip]
First of all, even while you state:
the only exception to this is the "commit/dispose" flag. Playing otherwise may lead to subtle errors.
In your case my_set should be also sent be reference, and if it's passed by value, you'll get the mentioned subtle errors (you will delete from the copy of the set, and the original set will be unchanged).
So you're just switching from one set of subtle errors to another one.
I'm not switching to any other errors if I have to specify explicitly whether the given argument is passed by reference or value.
But additionally to this another set of subtle errors you also break strong exception guarantee.
I'm not, if I know that the copied objects don't throw on ctor. Container iterators are common example of such.
However, there are cases when copy ctor can fail, but assignment (or some other form of initialization/copying) can't (for example, ctor of std::vector can throw, but if you created and reserved enough memory successfully, assignment will not throw as there is no allocation). So you need to create a temporary object before the insertion, and then initialize it in some no-throw fashion.
In this particular case I would, of course, try to do as you propose. But I can't that the case is common (at least, in my practice).
To conclude, I don't think it worth to make default an option that breaks strong exception guarantee, if there is one that doesn't.
I'm still not convinced. Once again, I did not advocate to pass arguments by value as a default library behavior. Making scope guards actually exception-safe is a tricky task and I tend to think it's not the place to introduce _any_ default and non-obvious behavior. Up to this moment I like the syntax that Alexander proposed, specifically: BOOST_SCOPE_EXIT( byref(r) byval(v) ) or any variation of such, including Steven's proposal: BOOST_SCOPE_EXIT((cref c)(ref r)(val v)) -- Best regards, Andrey mailto:andysem@mail.ru
AMDG Andrey Semashev <andysem <at> mail.ru> writes:
I can. But this is a burden since it should be done for almost all args I ever pass to ScopeExit. And I can easily forget to do so and get subtle errors I pointed out before. So this is by no way should be the default behavior.
You can also get subtle errors if the default is pass by value. You will almost always want to pass at least one argument by reference--the one you modify. If you pass all arguments by value then you will end up only modifying the copy. In Christ, Steven Watanabe
Hello Steven, Thursday, August 23, 2007, 7:45:27 PM, you wrote:
AMDG
Andrey Semashev <andysem <at> mail.ru> writes:
I can. But this is a burden since it should be done for almost all args I ever pass to ScopeExit. And I can easily forget to do so and get subtle errors I pointed out before. So this is by no way should be the default behavior.
You can also get subtle errors if the default is pass by value. You will almost always want to pass at least one argument by reference--the one you modify. If you pass all arguments by value then you will end up only modifying the copy.
I have already answered to Maxim on this. The essence is that I'm not saying that args should be passed by value by default. I'm saying that user should state explicitly which argument is passed this or that way. -- Best regards, Andrey mailto:andysem@mail.ru
Steven Watanabe wrote:
AMDG
Andrey Semashev <andysem <at> mail.ru> writes:
Generally speaking, you are right here. But first, there are many objects that don't throw on copying. And second, it's not always possible to make a copy beforehand. See one of my previous posts for an example (inserting into container yelds an iterator that is to be used in the scope-exit block). So I think the best way is to leave the decision up to user.
I don't understand. Why can't you make a copy right before the scope exit block? I think that for ScopeExit, always passing by reference is correct. At the very least it should be the default.
Passing by reference definitely gets +1 from me. If possible, and does not uglify the syntax, making it default only would be ok. / Johan
Maxim Yanchenko <maximyanchenko <at> yandex.ru> writes:
I vote to accept the library into Boost.
Thanks.
A couple of general comments after reading other reviews.
1. The fact that the library takes all arguments by reference looks unavoidable. If it allowed passing by value (i.e. making copies) - the copy constructors can throw, and this exception will be thrown after you made the change to your container/whatever you'd like to rollback, and before you setup the rollback action. So you can't guarantee that your rollback action will be executed.
Very good point. It makes me think that ScopeExit and local functions have different defaults for passing arguments. Agruments should be passed by value to a local function to avoid dangling references when the function outlives the scope. So, my current favorite is BOOST_SCOPE_EXIT( cref(c) ref(r) val(v) ). IMO, it looks more natural than (a)(b)(c). More verbose but this verbosity has a purpose.
So, in case you need a copy - it's much safer to create this copy before making the change to your container/whatever. It will also express your intention more clearly.
I should document it. -- Alexander
AMDG Alexander Nasonov <alnsn <at> yandex.ru> writes:
So, my current favorite is BOOST_SCOPE_EXIT( cref(c) ref(r) val(v) ). IMO, it looks more natural than (a)(b)(c). More verbose but this verbosity has a purpose.
I prefer BOOST_SCOPE_EXIT((cref c)(ref r)(val v)) for two reasons. 1) It makes it clear that they are /not/ function calls 2) It's slightly easier to implement because you can use BOOST_PP_SEQ_FOR_EACH. In Christ, Steven Watanabe
participants (5)
-
Alexander Nasonov -
Andrey Semashev -
Johan Nilsson -
Maxim Yanchenko -
Steven Watanabe