Move semantics:
The makeScopeGuard() function accepts the lambda by r-value ref; but passes a normal ref to the constructor of ScopeGuard.
template <typename Lambda>
ScopeGuard<Lambda> makeScopeGuard(Lambda&& func)
{
return ScopeGuard<Lambda>(func);
}
You should either update ScopeGuard to take the function by r-value ref or change the makeScopeGuard() take a normal ref (to be consistent).
Personally I would pass the r-value ref through to the ScopeGuard.
template <typename Lambda>
ScopeGuard<Lambda> makeScopeGuard(Lambda&& func)
{
return ScopeGuard<Lambda>(std::move(func)); // Need the move here
// a named value is not an r-value ref.
}
Also need to update ScopeGuard
ScopeGuard(Lambda&& func)
: committed_(false) // Please one line per variable
// Just like in normal code
// were you declare one variable per line.
, valid_(true)
, rollbackFunc_(std::move(func)) // Need the move again.
{}
No Except
Normally the move operators are marked noexcept. Unless you have a good reason not to I would continue that norm.
ScopeGuard(ScopeGuard&& other) noexcept
ScopeGuard& operator=(ScopeGuard&& other) noexcept
Move by swapping.
The easiest way to move something is to swap it.
// Now we could implement this with a swap.
// But this seems trivial enough So this is fine.
ScopeGuard(ScopeGuard&& other) noexcept
: committed_(other.committed_) // One variable per line please.
, valid_(other.valid_)
, rollbackFunc_(other.rollbackFunc_)
{
// Disabled the moved from.
other.valid_ = false;
}
ScopeGuard& operator=(ScopeGuard&& other) noexcept
{
// Here we are doing a copy (why not invoke the copy constructor)?
// Followed by marking it `false`.
committed_ = other.committed_;
valid_ = other.valid_;
rollbackFunc_ = other.rollbackFunc_;
other.valid_ = false;
return *this;
}
I would write like this:
I personally think it is easier to read like this.
ScopeGuard(ScopeGuard&& other) noexcept
: committed_(false)
, valid_(false)
, rollbackFunc_([](){})
{
swap(other);
}
ScopeGuard& operator=(ScopeGuard&& other) noexcept
{
swap(other);
other.valid_ = false;
return *this;
}
void swap(ScopeGuard& other) noexcept
{
using std::swap;
swap(committed_, other.committed_);
swap(valid_, other.valid_);
swap(rollbackFunc_, other.rollbackFunc_);
}
Abuse of mutable
mutable bool committed_;
Not sure I like this. Is this because you have a const value and want to get around the constness of the object?
Mutable objects are meant to represent objects that are not part of the full state of the object. i.e. a cached or computed part of the state that can be re-calculated.
committed_ is very much part of the objects main state. So I would not mark it mutable.
That of course means you have to remove the const off the commit method.
void commit() { committed_ = true; }
^^^^^ removed const
It is changing the state so its not really a const method.
Commit
There seems to be no way to call commit!!!
SCOPE_EXIT([f](){ baz(f); })
// how do I call commit on the above?
// I could use my editor to find the line number, but thats very fragile.
// And probably not what you intended to do.
bar(5); // It worked no exception
// So here I would expect to call commit.
If you can not call commit() then you should remove it from the class.
I would just get rid of the macors. Then your users can do:
auto guard = makeScopeGuard([](){revertStuff();}
// DO WORK HERE
// WORK DONE
guard.commit(); // no exceptions so we commit and don't need to revert.
An example working with correct move semantics fo the pedants:
#include <cstdio>
#include <cstdlib>
#include <memory>
#include <functional>
#include <iostream>
class ScopeGuard
{
public:
ScopeGuard(std::function<void()>&& func)
: committed_(false)
, valid_(true)
, rollbackFunc_(std::move(func))
{}
~ScopeGuard() noexcept
{
if (valid_ && !committed_)
rollbackFunc_();
}
ScopeGuard(const ScopeGuard&) = delete;
ScopeGuard& operator=(const ScopeGuard&) = delete;
ScopeGuard(ScopeGuard&& other) noexcept
: committed_(false)
, valid_(false)
{
swap(other);
}
ScopeGuard& operator=(ScopeGuard&& other) noexcept
{
swap(other);
return *this;
}
void swap(ScopeGuard& other)
{
using std::swap;
swap(committed_, other.committed_);
swap(valid_, other.valid_);
swap(rollbackFunc_, other.rollbackFunc_);
}
void commit() const { committed_ = true; }
private:
mutable bool committed_;
bool valid_;
std::function<void()> rollbackFunc_;␣
};
ScopeGuard makeScopeGuard(std::function<void()>&& func)
{
return ScopeGuard(std::move(func));
}
#define MY_CONCATENATE_NX(A, B) A ## B
#define MY_CONCATENATE(A, B) MY_CONCATENATE_NX(A, B)
#define SCOPE_EXIT(f) auto MY_CONCATENATE(EXIT, __LINE__) = makeScopeGuard(f);
int foo(const char*) noexcept {std::cout << "food\n";return 0;}
bool bar(int) noexcept {std::cout << "bar\n";return true;}
void baz(int) noexcept {std::cout << "baz\n";}
int main() {
int f = foo("/dev/random");
if (!f)
{
return EXIT_FAILURE;
}
SCOPE_EXIT([f](){ baz(f); })
int b = 1;
bar(b);
baz(f)
}
baz()It can see thatbar()isnoexcptandcommit()is not called. So the rest of the code can be removed. \$\endgroup\$rollbackFunc_ = other.rollbackFunc_;inScopeGuard& operator=(ScopeGuard&& other)is illegal. Error C3853 '=': re-initializing a reference or assignment through a reference-to-function is illegal \$\endgroup\$