7
\$\begingroup\$

I am developing a simple macro that enables Python style 'with' in C++. I have shamelessly prefixed the macro name with BOOST_ - primarily because there's a similarity to BOOST_FOREACH, and because I didn't want to call it plain WITH.

The motivation are familiar bugs like these:

void bug1() {
    std::unique_lock<std::mutex>(my_mutex);
    // oooops, forgot to give the lock a name
    function_call();
}

void bug2() {
    std::unique_lock<std::mutex> waldo(my_mutex);
    function_call();
    // oooops, forgot to unlock
    other_function_call();
}

void ugly() {
    {
        // intent of explicit scope not as obvious as could be
        std::unique_lock<std::mutex> waldo(my_mutex);
        function_call();
    }
    other_function_call();
}

So here are the questions:

  • Is this useful to anyone but me?
  • Is this a bad idea for whatever reason?
  • Have I overlooked a possibility to implement this in C++03?
  • Can it be implemented without the is_move_constructible type requirement?
  • Do you have suggestions for improvement?
  • (With a twinkle) Should I apply for some speaking time during the C++Now 2015 Lightning Talks and present this?

And here's my implementation:

NOTE: I have considered an alternative signature such as...

BOOST_WITH(type, ...)

...where ... would be the arguments to the constructor. I have decided to stick to this:

#ifndef BOOST_WITH_HPP_INCLUDED
#define BOOST_WITH_HPP_INCLUDED

#include <utility>
#include <type_traits>

// This macro expands to code that can be used in the same way as the standard
// control structures can. Whatever 'exp' returns lives as long as a loop
// variable would in similar context.
//
// Example:
// BOOST_WITH(std::unique_lock<std::mutex>(my_mutex))
//     do_something();
//
// Example:
// BOOST_WITH(Pushed_matrix()) {
//     draw_something();
//     draw_something_else();
// }
#define BOOST_WITH(exp)                                                        \
    if (auto BOOST_WITH_always_true = boost::with_detail::make_true(exp))


namespace boost {
namespace with_detail {

// wraps an object of movable type
// and provides conversion to bool (always true)
template <class T>
struct always_true {
    explicit always_true(T what) : x{std::move(what)} {}
    constexpr operator bool() const { return true; }
    T x;
};

// always_true<T> construction helper
template <class T>
always_true<T> make_true(T&& what) {
    static_assert(std::is_move_constructible<T>::value,
                  "BOOST_WITH requires the scoped object's type to be move "
                  "constructible");
    return always_true<T>{std::forward<T>(what)};
}

}} // namepace boost::with_detail


#endif // BOOST_WITH_HPP_INCLUDED

Finally, a minimal example:

#include "with.hpp"
#include <cassert>
#include <mutex>

// nothing serious, just a prove of concept
struct Lockable {
    Lockable() : locked(false) {}
    void lock() { locked = true; }
    void unlock() { locked = false; }
    bool locked;
};

int main() {
    Lockable the_lock;
    assert(!the_lock.locked);
    BOOST_WITH(std::unique_lock<Lockable>(the_lock))
        assert(the_lock.locked);
    assert(!the_lock.locked);
}
\$\endgroup\$
8
  • 2
    \$\begingroup\$ can you elaborate on probably yes (bad idea)? \$\endgroup\$
    – idefixs
    Apr 10, 2015 at 16:31
  • \$\begingroup\$ Do you know about stuff like std::lock_guard()?. This is probably a better a approach than using a macro. Mots of the time I consider writing macros is a bad idea, and will be hard to get completely right without any side effects. \$\endgroup\$ Apr 10, 2015 at 16:33
  • 2
    \$\begingroup\$ Yes I do. Read my post, especially the function void bug1() \$\endgroup\$
    – idefixs
    Apr 10, 2015 at 16:35
  • \$\begingroup\$ // oooops, forgot to give the lock a name And the compiler error message isn't pointing out this? \$\endgroup\$ Apr 10, 2015 at 16:37
  • 1
    \$\begingroup\$ The 'final' version, including suggestions by 5gon12eder is at 26elf.at/2015/04/python-style-with-in-c \$\endgroup\$
    – idefixs
    Apr 13, 2015 at 11:31

4 Answers 4

6
\$\begingroup\$

[ Foreword: Note that I'm not going to argue whether or not it is a good idea to invent a new macro-based syntax for this. I'm only reviewing your code. — end foreword ]

I think you have been pretty careful and cannot spot any serious issues with your implementation; nice job.

The only thing that I think could be improved is the error reporting. Your static_assertion fires much too late in the instantiation process such that the error message will end up buried under loads of template error messages. You can re-structure the instantiation machinery to improve this.

First, provide two overloads of boost::with_detail::make_true:

template <class T>
std::true_type make_true(T&&, std::false_type) {
    static_assert(std::is_move_constructible<T>::value,
                  "BOOST_WITH requires the scoped object's type to be move"
                  " constructible");
    return std::true_type{};  // never executed
}

template <class T>
always_true<T> make_true(T&& what, std::true_type) {
    static_assert(std::is_move_constructible<T>::value, "this is a bug");
    return always_true<T>{std::forward<T>(what)};
}

The static_assertion in the second overload is of course only paranoia and could be omitted.

Second, select the appropriate overload via tag dispatch:

#define BOOST_WITH(RESOURCE)                                        \
  if (auto BOOST_WITH_always_true = boost::with_detail::make_true(  \
      RESOURCE,                                                     \
      std::is_move_constructible<decltype(RESOURCE)>{}))

With this change, if I try to abuse your macro with a wrong type, GCC gives me this pretty straight-forward error message:

In file included from main.cpp:1:0:
boost/with.hpp: In instantiation of ‘std::true_type boost::with_detail::make_true(T&&, std::false_type) [with T = std::mutex; std::true_type = std::integral_constant<bool, true>; std::false_type = std::integral_constant<bool, false>]’:
main.cpp:22:5:   required from here
boost/with.hpp:42:7: error: static assertion failed: BOOST_WITH requires the scoped object's type to be move constructible
       static_assert(std::is_move_constructible<T>::value,

Compare this with what we would have got before.

Note that we cannot simply put static_assert(false, …); inside the make_true body because it would fire unconditionally on every compilation. It is necessary to make the asserted condition dependent on the template argument. Of course, any other template instantiated with T that always yields false would do equally well but using std::is_move_constructible (again) seems easiest.

\$\endgroup\$
1
  • \$\begingroup\$ I had noticed the late static assert, but was unable to come up with an elegant solution. Thank you! \$\endgroup\$
    – idefixs
    Apr 10, 2015 at 21:35
1
\$\begingroup\$

Abstraction in C++ is not a bad thing, neither does making the code more fluent, readable and (oh my) even fun!

my only answer for your question is : do we really need to imitate different languages inside our code when C++ already has what we need?

template <class retType , class... Args>
retType ensureSynchronized (retType(*givenFunction)(Args...) , Args... args){
         std::lock_guard<std::mutex> synchLock (somePredefinedMutex);
         return givenFunction(std::forward<Args>(args)...);
}

example:

int main(void){

try{
   ensuereSynchronized (myFunc, 1 ,ofstream("myFile"),someEnum::someValue);
}
catch{
  //cleanups
}
return 0;
}

isn't it cleaner , simpler AND solve all of your bugs? (plus does not use boost but the standard)

\$\endgroup\$
8
  • \$\begingroup\$ Besides not passing 'somePredefinedMutex' it is nice. \$\endgroup\$ Apr 10, 2015 at 17:23
  • \$\begingroup\$ this is not a complete code , it's just to illustrate what can be written with the language itself \$\endgroup\$
    – David Haim
    Apr 10, 2015 at 17:35
  • \$\begingroup\$ I like the concept. However, it's not very generic. You would pretty much have to write such a function template for each use case... \$\endgroup\$
    – idefixs
    Apr 10, 2015 at 17:49
  • \$\begingroup\$ @idefixs or you could use a lambda \$\endgroup\$
    – Mgetz
    Apr 10, 2015 at 18:12
  • \$\begingroup\$ I have thought of that. But that would be much like the explicit scope example, wouldn't it? Would it resolve the bug1() problem? Do you have something to show that it does? \$\endgroup\$
    – idefixs
    Apr 10, 2015 at 18:16
1
\$\begingroup\$

So here are the questions:

•Is this useful to anyone but me?

I wouldn't use it; It is basically copying/generalizing the functionality of a scoped lock (alter another object's state, while in scope) - and I would prefer a simple RAII wrapper for that (because it's expected, straightforward and doesn't write code using macros).

•Is this a bad idea for whatever reason?

Writing code using macros is usually a brittle solution (that is, "it works and it's fine" - as long as the code needs no maintenance).

The original code (the examples you give of symptomatic errors) should be fixed (in my opinion) according to SRP.

Your example (with my comments documenting my assumptions):

void bug2() {
    std::unique_lock<std::mutex> waldo(my_mutex); // scoped lock
    function_call();                              // should be synchronized
    other_function_call();                        // should NOT be synchronized
}

Conceptually, bug2 performs two types of operations (operates at different abstraction levels):

  • handle synchronization of function_call
  • perform sequence of two function calls

If you extract the first operation to a dedicated function:

// perform safe "function_call()"
// and _do nothing else_
void safe_function_call()
{
    std::unique_lock<std::mutex> waldo(my_mutex); // scoped lock
    function_call();                              // synchronized 
}

void bug2() // note: rename function, as it is no longer a bug
{
    safe_function_call(); // synchronized call is now an atomic operation
                          // as far as client code is concerned
    other_function_call();
}

This provides a better abstraction level separation and better primitives in your client code.

If you hide the existence of function_call in a separate compilation unit or protected/private area of a class, you create a situation where the original bug can no longer be replicated (anywhere) because you only allow for the safe alternative.

\$\endgroup\$
1
  • \$\begingroup\$ If the bug2() function were perfect, I would not have called it bug2. \$\endgroup\$
    – idefixs
    Apr 13, 2015 at 16:18
1
\$\begingroup\$

If anyone else finds this, such a macro can be greatly simplified in C++17:

#define WITH(exp) if ([[maybe_unused]] auto WithVarName = exp; true)
\$\endgroup\$
1
  • \$\begingroup\$ true. however, it suffers the same flaws that my original version did. move-only and dangling-else just to name the topics. updated version is at github.com/maysl/BOOST_WITH \$\endgroup\$
    – idefixs
    Sep 25, 2017 at 12:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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