scope exit something wrong?

Question

Is there something wrong with this code?

#pragma once
#ifndef SCOPEEXIT_HPP
# define SCOPEEXIT_HPP

#include <utility>

/* This counts the number of args */
#define NARGS_SEQ(_1, _2, _3, _4, _5, _6, _7, _8, _9, _10, N, ...) N
#define NARGS(...) NARGS_SEQ(__VA_ARGS__, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1)

/* This will let macros expand before concating them */
#define PRIMITIVE_CAT(x, y) x ## y
#define CAT(x, y) PRIMITIVE_CAT(x, y)

/* This will pop the last argument off */
#define POP_LAST(...) CAT(POP_LAST_, NARGS(__VA_ARGS__))(__VA_ARGS__)
#define POP_LAST_1(x1)
#define POP_LAST_2(x1, x2) x1
#define POP_LAST_3(x1, x2, x3) x1, x2
#define POP_LAST_4(x1, x2, x3, x4) x1, x2, x3
#define POP_LAST_5(x1, x2, x3, x4, x5) x1, x2, x3, x4
#define POP_LAST_6(x1, x2, x3, x4, x5, x6) x1, x2, x3, x4, x5
#define POP_LAST_7(x1, x2, x3, x4, x5, x6, x7) x1, x2, x3, x4, x5, x6
#define POP_LAST_8(x1, x2, x3, x4, x5, x6, x7, x8) x1, x2, x3, x4, x5, x6, x7
#define POP_LAST_9(x1, x2, x3, x4, x5, x6, x7, x8, x9) x1, x2, x3, x4, x5, x6, x7, x8
#define POP_LAST_10(x1, x2, x3, x4, x5, x6, x7, x8, x9, x10) x1, x2, x3, x4, x5, x6, x7, x8, x9

/* This will return the last argument */
#define LAST(...) CAT(LAST_, NARGS(__VA_ARGS__))(__VA_ARGS__)
#define LAST_1(x1) x1
#define LAST_2(x1, x2) x2
#define LAST_3(x1, x2, x3) x3
#define LAST_4(x1, x2, x3, x4) x4
#define LAST_5(x1, x2, x3, x4, x5) x5
#define LAST_6(x1, x2, x3, x4, x5, x6) x6
#define LAST_7(x1, x2, x3, x4, x5, x6, x7) x7
#define LAST_8(x1, x2, x3, x4, x5, x6, x7, x8) x8
#define LAST_9(x1, x2, x3, x4, x5, x6, x7, x8, x9) x9
#define LAST_10(x1, x2, x3, x4, x5, x6, x7, x8, x9, x10) x10

namespace detail
{

template <typename T>
class scope_exit
{
public:
  explicit scope_exit(T&& f) : f_(std::move(f)) { }

  scope_exit(scope_exit&& other) : f_(std::move(other.f_)) { }

  ~scope_exit() { f_(); }

private:
  T const f_;
};

class scope_exit_helper { };

template<typename T>
inline scope_exit<T> make_scope_exit(T&& f)
{
  return scope_exit<T>(std::forward<T>(f));
}

template<typename T>
inline scope_exit<T> operator+(scope_exit_helper&&, T&& f)
{
  return scope_exit<T>(std::forward<T>(f));
}

}

#define SCOPE_EXIT(...) auto const CAT(scope_exit_, __LINE__)\
  (::detail::make_scope_exit([POP_LAST(__VA_ARGS__)]{LAST(__VA_ARGS__);}))
#define SCOPE_EXIT2(...) auto const CAT(scope_exit_, __LINE__)\
  =::detail::scope_exit_helper()+[__VA_ARGS__]

#endif // SCOPEEXIT_HPP

Example usage:

{
  int* i(new int);

  SCOPE_EXIT(i, delete i);
  //SCOPE_EXIT2(i){delete i;};

  *i = 10;
}

Could something be improved?

Answer 1

Do you suppose to use this class as a member of others? if not (my guess), why to have move ctor? if yes, why there is no move assign? anyway being an other class member it will make latter non-copyable. It is quite Ok in some cases, but there are other cases possible as well -- some resources could be copied (like memory blocks), or shared (with refs count) -- anyway for class members, logic could be complicated, and there is no a silver bullet suitable for all possible members... so I think it would be better to restrict movability of this class and make it usable in function(members) context...

moreover, one more option is to disable operator new and operator new[] as well -- there is no reason to place scope_exit on heap (better to use smart pointers w/ custom deleter)...

and finally, I hate macros, but in this case a macro can make it slightly easy to use/type:

SCOPE_EXIT(i) { /* blah-blah */ }

it's quite possible (and easy to implement). Implementation may looks like this:

#define SCOPE_EXIT(...) \
  detail::scope_exit BOOST_PP_CAT(scope_exit_var_, __LINE__) = [__VA_ARGS__]()

BTW, to make it works, scope_exit ctor shouldn't be explicit. Usage example:

int socket = ...;
std::ofstream os(...);
std::ifstream is(...);
SCOPE_EXIT(=, &os, &is)
{
  close(socket);
  os.flush().close();
  is.close();
};

Answer 2

Not sure why you would want that?

If it is just to delete pointers then you can use a smart pointer:

int main()
{
    std::unique_ptr<int>  u(new int);
}

If you want to run arbitrator code. The same can be achieved using labdas and a smart pointer.

int main()
{
    std::shared_ptr<int>  s(new int, [](int*& ptr){delete ptr;std::cout << "Done";});
}

Using the example for boost provided below:

This looks like a nice example:

void world::add_person(person const& a_person) {
    bool commit = false;

    persons_.push_back(a_person);           // (1) direct action
    // Following block is executed when the enclosing scope exits.
    BOOST_SCOPE_EXIT(&commit, &persons_) {
        if(!commit) persons_.pop_back();    // (2) rollback action
    } BOOST_SCOPE_EXIT_END

    // ...                                  // (3) other operations

    commit = true;                          // (4) disable rollback actions
}

But I have to say I think "normall" RAII provides a better solution:

void world::add_person(person const& a_person)
{
    Transaction   trans(persons);

    trans.push_back(a_person);             // (1) direct action


    // No action required.                // (2) rollback action
    // If a transaction is not explicitly commited
    // Then a destructor is going to forece
    // rollback on the transaction. 


    // ...                                // (3) other operations

    trans.commit();                       // (4) disable rollback actions
}

Much easier and more intuitive to read.

Answer 3

You can also get:

  int* i = new int();

  SCOPE_EXIT
  {
      delete i;
  };

  *i = 10;

With the following implementation:

namespace detail 
{
    template<typename T>
    class scope_exit
    {
        scope_exit(const scope_exit&);
        scope_exit& operator=(const scope_exit&);
    public:         

        template<typename T2>
        explicit scope_exit(T2&& func) 
            : func_(std::forward<T2>(func))
            , valid_(true)
        {
        }

        scope_exit(scope_exit&& other)
            : func_(std::move(other.v))
            , valid_(std::move(other.valid_))
        {
            other.valid_ = false;
        }

        scope_exit& operator=(scope_exit&& other)
        {
            func_  = std::move(other.func_);
            valid_ = std::move(other.valid_);

            other.valid_ = false;

            return *this;
        }

        ~scope_exit()
        {
            try
            {
                if(valid_)
                    func_();
            }
            catch(...)
            {
                LOG_CURRENT_EXCEPTION();
            }
        }
    private:
        T func_;
        bool valid_;
    };          

    class scope_exit_helper {};

    template <typename T>
    scope_exit<typename std::decay<T>::type> operator+(scope_exit_helper, T&& exitScope)
    {
        return scope_exit<typename std::decay<T>::type>(std::forward<T>(exitScope));
    }
}

#define _EXIT_SCOPE_LINENAME_CAT(name, line) name##line
#define _EXIT_SCOPE_LINENAME(name, line) _EXIT_SCOPE_LINENAME_CAT(name, line)
#define SCOPE_EXIT auto _EXIT_SCOPE_LINENAME(EXIT, __LINE__) = ::detail::scope_exit_helper() + [&]() mutable