Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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))
  {
    static_assert(noexcept(f_()), "throwing functors are unsupported");
  }

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

  ~scope_exit() { f_(); }

private:
  T 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__)]() mutable noexcept\
    { LAST(__VA_ARGS__); }))
#define SCOPE_EXIT_ auto const CAT(scope_exit_, __LINE__)\
  =::detail::scope_exit_helper()+[&]() mutable noexcept
#define SCOPE_EXIT__(...) auto const CAT(scope_exit_, __LINE__)\
  =::detail::scope_exit_helper()+[__VA_ARGS__]() mutable noexcept

#endif // SCOPEEXIT_HPP

Example usage:

int main()
{
  {
    int* i(new int);

    SCOPE_EXIT(i, delete i);

    *i = 10;
  }

  {
    int* i(new int);

    SCOPE_EXIT_{ delete i; };

    *i = 11;
  }

  {
    int* i(new int);

    SCOPE_EXIT__(i){ delete i; };

    *i = 12;
  }

  return 0;
}

Could something be improved?

share|improve this question

3 Answers 3

up vote 3 down vote accepted

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();
};
share|improve this answer
    
But how to generate a unique name for the scope_exit variable in the SCOPE_EXIT macro? It needs to have a name, or it gets destroyed before scope is exited. –  user1095108 Dec 18 '12 at 16:30
    
using __LINE__ in its name for example... –  zaufi Dec 18 '12 at 16:47
    
code fixed, keep the suggestions coming in. –  user1095108 Dec 19 '12 at 9:05
    
I suggest lambda body is not a part of macro... I.e. I supposed exactly that I'm typed as example... -- it looks better than you version... (less brackets) –  zaufi Dec 19 '12 at 15:48
    
Can't do it :( If you know of some better code, please post an url, or write your own code and post an url. –  user1095108 Dec 20 '12 at 9:32

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.

share|improve this answer
    
this is mostly not for pointers and shared_ptr has overhead and can be copied which is definitely unfavourably for exit_scope –  zaufi Dec 19 '12 at 15:45
2  
@zaufi: I see no other use for it (other than a bad option of ptr clean-up). Standard RAII allows you to execute arbitrary code. I see no need to wrap this in one more layer of macros and object creation. Frankly I see absolutely no benefit to the code. conclusion throw it away and use standard techniques (so that people understand what is happening). –  Loki Astari Dec 19 '12 at 16:34
    
read here about reasons: boost.org/doc/libs/1_52_0/libs/scope_exit/doc/html/index.html –  zaufi Dec 19 '12 at 16:46
2  
@zufi: I think that is a terrible example. RAII under normal curcumstances provides a much cleaner and more readable way to achieve the same affect. This SCOPE_EXIT seems to be cool when you are using a C interface which just means you have not put the effort into creating a clean C++ OO interface (as a result it seems like a hack to get around doing the work you should be doing (designing a clean OO interface)). –  Loki Astari Dec 20 '12 at 17:56
1  
@user1095108: This is still a bad example. Anywhere you write SCOPE_EXIT{STUFFF} You can write class BLA { STUFF } and have a cleaner interface. If you can come up with a concrete example and I can't build a cleaner implementation using a class than we can talk otherwise you are inventing a solution to a problem that does not exist in my belief. –  Loki Astari Dec 21 '12 at 18:40

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
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.