3
\$\begingroup\$

I'm a fanboy of the Boost library and I've used Boost's components extensively in my C++03 project. The team decided to merge several pieces of code including my project. I will get the C++14 environment, but I have to drop the Boost library, so I'd like to replace BOOST_SCOPE_EXIT with a modern C++ technique.

Here is SCOPE_EXIT's implementation, with a short example:

#include <cstdio>
#include <cstdlib>
#include <memory>

template <typename Lambda>
class ScopeGuard
{
public:
    ScopeGuard(const Lambda& func)
        : committed_(false), valid_(true), rollbackFunc_(func) {}

    ~ScopeGuard() noexcept
    {
        if (valid_ && !committed_)
            rollbackFunc_();
    }

    ScopeGuard(const ScopeGuard&) = delete;
    ScopeGuard& operator=(const ScopeGuard&) = delete;

    ScopeGuard(ScopeGuard&& other)
        : committed_(other.committed_), valid_(other.valid_), rollbackFunc_(other.rollbackFunc_)
    {
        other.valid_ = false;
    }

    ScopeGuard& operator=(ScopeGuard&& other)
    {
        committed_ = other.committed_;
        valid_ = other.valid_;
        rollbackFunc_ = other.rollbackFunc_;
        other.valid_ = false;
        return *this;
    }

    void commit() const { committed_ = true; }

private:
    mutable bool committed_;
    bool valid_;
    Lambda rollbackFunc_; 
};

template <typename Lambda>
ScopeGuard<Lambda> makeScopeGuard(Lambda&& func)
{
    return ScopeGuard<Lambda>(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;
bool bar(int) noexcept;
void baz(int) noexcept;

int main() {
    int f = foo("/dev/random");
    if (!f)
    {
        return EXIT_FAILURE;
    }
    SCOPE_EXIT([f](){ baz(f); })
    int b = 1;
    bar(b);
    // baz(f)

    return EXIT_SUCCESS;
}

Godbolt gcc 7.2 -std=c++14 -O2

VS2017 -W4 /EHsc /O2

gcc's assembly is pretty short, however MSVC's assembly is so long (about 70 lines) and I can't understand what's going on. (I am not familiar with the Windows environment.)

Did I miss something, or is it a bad optimization of MSVC?

\$\endgroup\$
  • \$\begingroup\$ gcc didn't compile any of the ScopeGuard code. \$\endgroup\$ – Millie Smith Nov 9 '17 at 5:53
  • \$\begingroup\$ @MillieSmith Then that is absolute good optimization. Don't you think? \$\endgroup\$ – Byoungchan Lee Nov 9 '17 at 5:59
  • \$\begingroup\$ @ByoungchanLee, I believe better version was already shown during Andrei Alexandrescu's talk, I belive in cppcon 2014. I implemented it, made nano modifications, and then posted here. \$\endgroup\$ – Incomputable Nov 9 '17 at 6:06
  • \$\begingroup\$ @MillieSmith It has to put the destructor somewhere (it has a side affect). The rest of the code can be elided but it still needs to call baz() It can see that bar() is noexcpt and commit() is not called. So the rest of the code can be removed. \$\endgroup\$ – Martin York Nov 9 '17 at 17:40
  • \$\begingroup\$ rollbackFunc_ = other.rollbackFunc_; in ScopeGuard& operator=(ScopeGuard&& other) is illegal. Error C3853 '=': re-initializing a reference or assignment through a reference-to-function is illegal \$\endgroup\$ – phy nju Dec 17 '17 at 17:55
4
\$\begingroup\$

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)    
}
\$\endgroup\$
  • \$\begingroup\$ I believe ut should be std::exchange() for move constructor, as it better matches semantics \$\endgroup\$ – Incomputable Nov 9 '17 at 17:10
  • \$\begingroup\$ Example in documentation uses it to implement move constructor \$\endgroup\$ – Incomputable Nov 9 '17 at 17:20
  • \$\begingroup\$ @Incomputable Interesting. I have not seen that before. Not yet convinced that it provides better semantics (or that it is more standard). I see it works well for small POD like objects. But when you start getting complicated members it will start to be less efficient (As you have to construct that member to put in place (less efficient as you already have that member constructed at destination)). Then you have a dichotomy in that some moves are done with swap() and others with exchange(). Seems much smaller maintenance cost to make the move semantics implemented the same way everywhere. \$\endgroup\$ – Martin York Nov 9 '17 at 17:33
  • \$\begingroup\$ I see what you mean. I agree on complications it brings. May be with time, as idioms will be more widespread, they will be better. If you're interested, here is 3.5 minute talk at cppcon on std::exchange() idioms. I believe it is the same swap, it just returns the value. \$\endgroup\$ – Incomputable Nov 9 '17 at 17:38
  • 1
    \$\begingroup\$ Actually, makeScopeGuard isn't accepting a rvalue reference, but a so-called forwarding reference (since Lambda gets deduced). This means that makeScopeGuard accepts both rvalue and lvalue references, so std::forward<Lambda>(func) should be called instead of std::move(func) (Otherwise, if called with a lvalue reference to a functor, the functor will be moved from, which very likely is not intended. E.g. auto f = std::function<void()>([&](){ doStuff();}); SCOPE_EXIT(f); /* f got moved from, so this call is not valid anymore: */ f();). \$\endgroup\$ – hoffmale Nov 9 '17 at 21:31
1
\$\begingroup\$

Making a copy of the lambda there is going to hurt you a little bit, at least in practice. Anyway, I strongly recommend using what I call "the Auto macro":

http://www.club.cc.cmu.edu/~ajo/disseminate/auto.h

int main() {
    int f = foo("/dev/random");
    if (!f) {
        return EXIT_FAILURE;
    }
    Auto(baz(f));
    int b = 1;
    bar(b);
    return EXIT_SUCCESS;
}

VS2017 generates perfect code, and as a (non-negligible) bonus, you end up with really nice syntax — no ugly [](){ ... } boilerplate. You just write the exact code you want to run — kind of like Go's defer baz(f); but even better (due to subtleties of eager evaluation in Go that aren't relevant here ;)).

\$\endgroup\$
  • \$\begingroup\$ This design is elegant. I want to fetch the object from Auto Macro, so that I can call its method later on. However I could not figure out how can I get the instance_, help please. @quuxplusone \$\endgroup\$ – phy nju Dec 16 '17 at 9:38
  • \$\begingroup\$ @phynju: If you ask a detailed question on StackOverflow and "@" my username, I'll try to help out. My suspicion right now is that you're trying to do something that is not Auto's use-case. If you just want a way to package up a behavior into an object you can "call later on," you're probably just looking for C++11 lambdas, or perhaps std::function, not a scoped syntactic construct like Auto(...);. \$\endgroup\$ – Quuxplusone Dec 16 '17 at 16:13
  • \$\begingroup\$ The main point of ScopeGuard is to branching exception and nonexception code. That is you will need to dismiss(or commit sometimes they call) when no-exception raised. I like the style Auto(fun(parameters)) you code support. however, any suggestions if to make it be able to be dismissed later on. \$\endgroup\$ – phy nju Dec 16 '17 at 23:04
  • \$\begingroup\$ @phynju: You mean, like, bool dismissed = false; Auto(if (!dismissed) do_something()); ... dismissed = true; ...? \$\endgroup\$ – Quuxplusone Dec 17 '17 at 5:11
  • \$\begingroup\$ Not exactly. add a dismiss method to the class void dismiss(){this.mDismiss=true;} and destructor ~AtScopeExit() { if(!mDismiss)m_lambda(); } , so then do this: auto obj = Auto(reversefun(...)); FollowUpOperation(); obj.dismiss(); So that when FollowUpOperation(); raised exception, the reversefun() will be called, otherwise, no. As I understand from ScopeGuard by Andrei Alexandrescu and Petru Marginean. So the problem is how can I get the object (obj here) created by Auto be refereed later. \$\endgroup\$ – phy nju Dec 17 '17 at 6:25
1
\$\begingroup\$

TL;DR: MSVC does some extra stuff and really doesn't like the call to makeScopeGuard.

To elaborate:

The main functions in the original examples only takes 16 instruction (GCC) or 23 (+8 inside makeScopeGuard) instructions (MSVC) respectively.

GCC inlined everything it could, proved that valid_ and committed_ don't change (so they are basically constant), elided the branch and variable allocations, and just calls baz(f) directly before exiting.

MSVC, however, only inlines the destructor of ScopeGuard into main and the constructor of ScopeGuard into makeScopeGuard. MSVC seems unable to inline the call to makeScopeGuard or to prove that valid_ and committed_ don't actually change values (so it has to create them on the stack and generate instructions/calls for them).

But this can be improved! Changing the macro so it doesn't call makeScopeGuard suddenly allows MSVC to come to the same conclusions as GCC and remove the checks and thus the stack allocations.

#define SCOPE_EXIT(f) auto MY_CONCATENATE(EXIT_lambda, __LINE__) = f;\
    auto MY_CONCATENATE(EXIT_var, __LINE__) =\
    ScopeGuard<decltype(MY_CONCATENATE(EXIT_lambda, __LINE__))>{std::move(MY_CONCATENATE(EXIT_lambda, __LINE__))};

MSVC still does produce some extra instructions (for the ScopeGuard's and the lambdas constructors and destructors), but those are never actually called. The final count for main of MSVC would be 19 instructions (only 3 more than GCC because it does some stack pointer management, though that could probably be turned off with the right compiler switch).

Link to godbolt with fixed macro (MSVC)

\$\endgroup\$
  • 1
    \$\begingroup\$ Working around the compiler is not a good way to do things. If the compiler is not generating perfect code it has a reason. Now it could be that its optimizations are not perfect yet. But they will get better over time. So it is best to write more readable code and let the compiler get better over time. Thus your code is always readable and thus maintainable and eventually the compiler will generate perfect code but will always generate correct code. \$\endgroup\$ – Martin York Nov 9 '17 at 17:43
  • 1
    \$\begingroup\$ @LokiAstari: My intention was not to "show how to beat the optimizer", but to "explain what the optimizer does". I'm with you that usually correct maintainable code is more important, but sometimes that extra edge in performance is needed. Also, it's interesting to try and understand why the compiler isn't able to do some optimizations (e.g. in order to find why your code might not be conforming). After all, the optimizer doesn't look at UB and correctness the way we do! \$\endgroup\$ – hoffmale Nov 9 '17 at 21:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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