General concept ACK on a RAII approach (C++ wrapper) for libevent.
I think we should avoid boost::noncopyable and instead use C++11's MyClass& operator=(const MyClass&) = delete; approach.

@jonasschnelli Fixed.

ACK on making things RAII. Thanks.

I'm not so sure about wrapping the entire interface, though. I'm not sure about others but I'd prefer something thinner.

For example evhttp_set_max_headers_size -> setMaxHeadersSize. This makes it fractionally harder for someone that has learned the libevent APIs to understand the code because the functions are named differently.

(also it adds maintenance overhead as every function of libevent to be used has to be wrapped)

@laanwj It felt sensible to make the underlying ev* structs private but maybe my intuition was off on that. I'm not entirely convinced that evhttp_set_max_headers_size -> setMaxHeadersSize is a problem, but if you think it is, it may be better to un-private'ify the structs. That would remove the need for friends as well.

It's not a problem but it's one of the little things that I've learned in years developing in open source. If a project makes use of some library which you already know, it's easier to get into it if it is simply using the "lexicon" of that library instead of wrapping everything in custom-named calls.

You could work around this by making the wrapping functions the same as the evhttp calls, e.g. don't do the camel-case thing. But yes I'd prefer not making the inner objects private - make it behave more like a "smart pointer".

Another (less-LoC) alternative to all of this is to define a std::unique_ptr with our own cleanup hook. I've done a similar thing recently in paymentserver.cpp.

struct X509StoreDeleter {
      void operator()(X509_STORE* b) {
          X509_STORE_free(b);
      }
};
std::unique_ptr<X509_STORE, X509StoreDeleter> certStore;

There is also a slight issue with the evhttp_request which needs to not be freed if a call to evhttp_make_request has been made. By exposing the internal evhttp_request, it's not possible to determine which is the case.

That (names being same) makes sense. The unique_ptr approach looks very nice, but unsure if it can address the above issue adequately.

There is also a slight issue with the evhttp_request which needs to not be freed if a call to evhttp_make_request has been made. By exposing the internal evhttp_request, it's not possible to determine which is the case.

Couldn't, in that case, the calling code could just .release() the pointer? No need to build that intelligence into the pointer object.

You mean, explicitly tell the wrapper it no longer owns the object whenever you do evhttp_make_request? I suppose that is doable. I would have loved to not have to risk developers forgetting to do so though.

Yeah, that makes sense. Thanks for the feedback, will fix!

@laanwj I decided to go with the unique pointer + deleter approach. It became awfully verbose, so I did some macroing.

Nice!
Concept ACK. I think the use of macros is okay in this case, as indeed they all follow the same pattern and it saves a lot of dumb repetition.

I tried building this and it fails:

$ make
Making all in src
make[1]: Entering directory '/store2/build/bitcoin/bitcoin/src'
make[2]: Entering directory '/store2/build/bitcoin/bitcoin/src'
...
  CXX      test/test_test_bitcoin-raii_event_tests.o
/.../bitcoin/src/test/raii_event_tests.cpp:5:10: fatal error: 'event2/event.h' file not found
#include <event2/event.h>
         ^~~~~~~~~~~~~~~~
1 error generated.

Perhaps the include directory for libevent headers is not specified for the tests in the Makefile.am?

@laanwj Weird. What does V=1 make give as output?

Edit: Did see that $(EVENT_CFLAGS) was not added to test_test_bitcoin_CPPFLAGS in Makefile.test.include (line 113). I'm confused why my machines (1 Mac OS X and 1 Ubuntu inside VirtualBox) and Travis both find the headers. Does it work if you add that?

Sure:

/usr/bin/ccache /opt/clang40/bin/clang++ -std=c++11 -DHAVE_CONFIG_H -I. -I.../bitcoin/src -I../src/config  -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I. -I./obj  -pthread -I/usr/include -I.../bitcoin/src/leveldb/include -I.../bitcoin/src/leveldb/helpers/memenv   -I.../bitcoin/src/secp256k1/include -I.../bitcoin/src/univalue/include -I./test/ -DBOOST_TEST_DYN_LINK -Qunused-arguments -ggdb -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -Wstack-protector -fstack-protector-all -fPIE -g -O2 -Wall -Wextra -Wformat -Wformat-security -Wno-unused-parameter -Wno-self-assign -Wno-unused-local-typedef -Wno-deprecated-register -MT test/test_test_bitcoin-raii_event_tests.o -MD -MP -MF test/.deps/test_test_bitcoin-raii_event_tests.Tpo -c -o test/test_test_bitcoin-raii_event_tests.o `test -f 'test/raii_event_tests.cpp' || echo '.../bitcoin/src/'`test/raii_event_tests.cpp
.../bitcoin/src/test/raii_event_tests.cpp:5:10: fatal error: 'event2/event.h' file not found
#include <event2/event.h>
         ^~~~~~~~~~~~~~~~
1 error generated.

My configure script looks like:

#!/bin/bash
CLANGPATH=/opt/clang40
$1/configure --with-incompatible-bdb \
    --prefix=/tmp/bitcoin \
    CC="${CLANGPATH}/bin/clang " CXX="${CLANGPATH}/bin/clang++" \
    OBJCXX="${CLANGPATH}/bin/clang++" \
    CPPFLAGS="-ggdb" \
    EVENT_CFLAGS="-I/opt/libevent/include" \
    EVENT_LIBS="-L/opt/libevent/lib -levent" \
    EVENT_PTHREADS_CFLAGS="-I/opt/libevent/include" \
    EVENT_PTHREADS_LIBS="-L/opt/libevent/lib -levent_pthreads" \

On my system, I have event2 symlinked inside /usr/local/include, which means I don't have to explicitly include it in the g++ call. Adding $(EVENT_CFLAGS) to test_test_bitcoin_CPPFLAGS added explicit references so this should fix it on your end.

Yep, that solved it, thanks! utACK 05a55a6