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);
}
4 Answers 4
[ 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_assert
ion 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.
-
\$\begingroup\$ I had noticed the late static assert, but was unable to come up with an elegant solution. Thank you! \$\endgroup\$idefixs– idefixs2015年04月10日 21:35:24 +00:00Commented Apr 10, 2015 at 21:35
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)
-
\$\begingroup\$ Besides not passing 'somePredefinedMutex' it is nice. \$\endgroup\$Dieter Lücking– Dieter Lücking2015年04月10日 17:23:30 +00:00Commented 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– David Haim2015年04月10日 17:35:24 +00:00Commented 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– idefixs2015年04月10日 17:49:05 +00:00Commented Apr 10, 2015 at 17:49
-
\$\begingroup\$ @idefixs or you could use a lambda \$\endgroup\$Mgetz– Mgetz2015年04月10日 18:12:33 +00:00Commented 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– idefixs2015年04月10日 18:16:12 +00:00Commented Apr 10, 2015 at 18:16
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.
-
\$\begingroup\$ If the
bug2()
function were perfect, I would not have called itbug2
. \$\endgroup\$idefixs– idefixs2015年04月13日 16:18:49 +00:00Commented Apr 13, 2015 at 16:18
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)
-
\$\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– idefixs2017年09月25日 12:03:21 +00:00Commented Sep 25, 2017 at 12:03
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\$void bug1()
\$\endgroup\$// oooops, forgot to give the lock a name
And the compiler error message isn't pointing out this? \$\endgroup\$