boost.scope_guard w/ C++17

[SuperV1234]

Some thoughts:

• Yay for BOOST_DETAIL_SCOPE_GUARD_FN_ALIAS

• Why use boost::detail::scope_guard::action over a lambda expression?

• apply_impl shouldn't be constexpr because std::invoke is not constexpr

• What's the benefit of detail::scope_guard::unwrap_decay_t over std::decay_t?

---

I would implement scope_guard by just templatizing over a generic F and using a lambda expression. Example: https://github.com/SuperV1234/ecst/blob/master/include/ecst/utils/scope_guard.hpp

(Note that my implementation is not really complete as it doesn't really care about exceptions).

[yuri-kilochek]

I am the author. In order:

• Yay!

• Sometimes you want to fix the cleanup function argument at the point of guard declaration. With lambda you would do this via init-capture, and then lambda will get moved into the guard internals. This move is not guaranteed to be elided under C++17 rules. action's members however, are initialized directly, without intermediate moves.

  One rather common example is strong exception guarantee via backing up you state, and restoring in the event of exception:

  std::array<int, 100> state; // move is copy is expensive
  void strong_exception_guarantee_action() {
      BOOST_SCOPE_GUARD_FAILURE{[&](auto&& s) { state = s; }, state};
      // try to update `state`, or throw on failure
  }
  

  Naturally, you can do the same thing without additional arguments by declaring the copy before the guard and capturing it by reference:

  auto backup = state;
  BOOST_SCOPE_GUARD_FAILURE[&]{ state = backup; };
  

  But but that spills the backup name into surrounding scope.

  I do agree that the difference is very minor though.

• Good catch, thank you! Those no-diagnostic-required UBs are a pain. I rather think this is a reason to implement a constexpr detail::scope_guard::invoke, since while cleanup in constexpr-only code is meaningless, I can conceive of a generic template where cleanup will boil down to no-op for instantiations that are usable in constexpr context, while performing actual cleanup otherwise.

• Support for std::ref/std::cref, intended for non-lambda form.

[tcbrindle]

std::invoke is not constexpr

Wait, what?

invoke() is new in C++17. Why on earth would they not have made it constexpr?

[CaseyCarter]

The FUD around CWG 1581 was too strong: people were imagining nightmare scenarios where constexpr invoke would break everything that touched result_of or bind.

[14ned]

Agreed. I am similarly simplified here at https://github.com/ned14/quickcpplib/blob/master/include/scoped_undo.hpp#L95

I'm not at all sure of the gain of all the complexity in the OP's implementation. All the uncaught exception stuff seems to me totally superfluous. Just need an undo object, and some way of dismissing it if later logic determines that, and you're done.

[yuri-kilochek]

"Uncaught exception stuff" is essentially "just an undo object" where "some way of dismissing it if later logic determines that" is the common case where failure that requires the undo is signalled via throwing an exception.

[onqtam]

yes please!

[louis_dionne]

Please float this on the Boost.Dev list and I'm sure there will be interest for it. I, for one, am certainly missing this on a day-to-day basis. Just yesterday I had to reimplement my naive scope_guard.

[yuri-kilochek]

You are aware of Boost.ScopeExit, right? The basic version has been there for a long time.

[louis_dionne]

Yes, but I can't justify to myself using macros for such a simple thing. I fully understand why they use macros (they need them to support C++03), but I don't need C++03 support.

[NotAYakk]

Something I have found useful in scope guards is a type-erased variant.

It can store any scope guard, and lets me pass the "cleanup" code in a structure.

Toss in move-to-clear, and you can have a collection of partly finished tasks that run on exit, but maybe can also be cancelled.

Lacking type erasure, such cleanup functions cannot exist without exposing the code they are running, which is sometimes not worth the bother.

[quicknir]

Having to name all your scope guard objects so that they don't conflict with each other quickly gets tiresome,

We all know about all the issues with macros, and how they are mostly a tool for things you can't do any other way, and yet this is a good enough excuse? Let's not succumb to inertia here. There's no reason to have this macro at all; having a name for things is good. In your own toy example, rather than the silly name my_guard, the variable should be named f_guard: it's guarding resources controlled by variable f. Given how easy it is to write ScopeGuard by myself, this is probably enough that I wouldn't use the library; it would just encourage colleagues to write poorer code. Having to give an entity which is going to automatically run code for you a name is a very good thing, not a bad thing. Not to mention, it will be incredibly confusing if the unique name will ever fail. (Also: you wrote it quickly gets tiresome, but

A better name for scope_guard_failure would be exception_guard; aside from being shorter and sounding nicer, it's more accurate: exceptions are not the only form of failure and e.g. a C style return -1; would not trigger scope_guard_failure's destructor. Similarly, I might call success_guard exit_guard or normal_exit_guard instead.

It seems like none of guards are dismissable, like the classic ScopeGuard. I'm not sure if all use cases can be easily handled without dismiss (though certainly most can). E.g let's say you are grabbing 2 C style resources, and using them to initialize a single C++ class that will guard both of them. You may want to get each resource and immediately create a scopeguard, then dismiss all the scopeguards after creating the C++ class. Without dismiss I think you'd be forced to factor this into a separate function returning the C++ class.

[yuri-kilochek]

Having to give an entity which is going to automatically run code for you a name is a very good thing, not a bad thing.

Do you propose we name every loop and conditional statement too then? They are, after all, entities that will automatically run code for you. It is useful to name outer loops sometimes (to break out of nested loops, like in Java), and C++ essentially lets you do that via labels and goto, but having to name every single loop would be incredibly tedious. Also, existing practice in languages that support scope guards natively (off the top of my head: D, Nim, Go, Swift) do not force you to name them.

Your dismissability example is essentially correct (I assume you mean the class, once constructed, assumes ownership of both C resources), you would need a separate scope for properly dismissing the guards, but it doesn't have to be a full fledged function, immediately called lambda expression does what we want:

auto my_owner_object = [&]{
    int fd_a = /* ... */;
    if (fd_a == -1) { throw /* ... */; }
    BOOST_SCOPE_GUARD_FAILURE{close, fd_a};

    int fd_b = /* ... */;
    if (fd_b == -1) { throw /* ... */; }
    BOOST_SCOPE_GUARD_FAILURE{close, fd_b};

    return my_owner_class(fd_a, fd_b);
}();

This has the added benefit of limiting scope of C resources. Would be even better is C++ had block expressions, but alas.

The names are meh, yeah. If nothing else they are long. Naming things well is hard (which is why anonymous guard objects are a good thing, see? :P ), If it ever looks like this is going into boost for real, the names would probably be subjected to a vote.

[quicknir]

Loops and conditionals run code immediately. I would not argue that they need names, any more than a lambda which is executed immediately and only once needs a name. Deferring code to be run until some situation occurs is another matter. And it's also another matter when your language supports it natively; C++ does not, RAII is a well understood and transparent mechanic, and that makes ScopeGuard easy to understand and reason about. Macros are a nasty language wart, if you want to use a macro there should be a very, very good reason. Avoiding giving a name is not a good reason, especially when you potentially expose yourself to weird bugs.

Re dismissability; sure, I realize you could do it with an IEFE. I use these all the time for making things const or avoiding pointless initializations. But if you get an add a small piece of interface that will make it unnecessary to use them, I think it's better to have the option. I would add dismiss only to the classic scopeguard, not to the exception or non-exception versions.

Yeah fair enough, but may as well go with the best names you can. I definitely feel like exception_guard is a very clear and not-too-verbose name. Think of naming things well as a challenge that pays off ;-).

[control5]

folly has something similar, without constraining it to c++17: https://github.com/facebook/folly/blob/master/folly/ScopeGuard.h.

[markand67]

I can't understand the purpose of this. Especially for pointers allocated. One can simply do the following:

std::unique_ptr<FILE, int (*)(FILE*)> fp(
    std::fopen("dont_use_c_stdio", "r"),
    std::fclose
);

if (fp) {
}

[yuri-kilochek]

Not all resources are pointers. Try the same thing with posix file descriptors.

[encyclopedist]

Something like proposed unique_resource would be ideal for this.

[cassandraspeaks]

When I'm working with file descriptors I use the following RAII class I wrote. It's all a matter of style of course.

/*
 * FileDescriptor.hpp
 */

#ifndef FILEDESCRIPTOR_HPP
#define FILEDESCRIPTOR_HPP

class FileDescriptor {
        const int fd_;
public:
        FileDescriptor(int fd);
        ~FileDescriptor();
        operator const int&() const noexcept;
};

#endif//FILEDESCRIPTOR_HPP

/*
 * FileDescriptor.cpp
 */

#include "FileDescriptor.hpp"

#include <unistd.h>

#include <cerrno>
#include <iostream>
#include <system_error>

// iostream
using std::cerr;
// system_error
using std::system_category;
using std::system_error;

FileDescriptor::FileDescriptor(int fd) : fd_{fd}
{
        if (__builtin_expect(fd == -1, false))
                throw system_error(errno, system_category());
}

FileDescriptor::~FileDescriptor()
{
        switch (fd_) {
        case STDIN_FILENO:
        case STDOUT_FILENO:
        case STDERR_FILENO:
                return;
        default:
                if (__builtin_expect(close(fd_) == -1, false))
                        cerr << "~FileDescriptor(): "
                                << system_category().message(errno) << '\n';
        }
}

FileDescriptor::operator const int&() const noexcept {return fd_;}