This PR is the initial part of cleaning up the use of ev*_new/ev*_free.
The relevant, owned types are wrapped in std::unique_ptrs with associated deleters.
Some cases require manually releasing the pointer, such as when an evhttp_request is handed to evhttp_make_request (as its ownership is moved at that point), but otherwise things become mostly automagic.
boost::noncopyable and instead use C++11’s MyClass& operator=(const MyClass&) = delete; approach.
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)
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.
0struct X509StoreDeleter {
1 void operator()(X509_STORE* b) {
2 X509_STORE_free(b);
3 }
4};
5std::unique_ptr<X509_STORE, X509StoreDeleter> certStore;
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.
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.
evhttp_make_request? I suppose that is doable. I would have loved to not have to risk developers forgetting to do so though.
True, though hiding behaviour in the wrapper instead of explicit transfer of ownership could also cause confusion, as there is a certain expectation from the API.
On Dec 20, 2016 11:46 AM, “kallewoof” notifications@github.com wrote:
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.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/9387#issuecomment-268212325, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHutu5cec1TevmA-izXHyoEfiUmitigks5rJ7IDgaJpZM4LRjq0 .
I tried building this and it fails:
0$ make
1Making all in src
2make[1]: Entering directory '/store2/build/bitcoin/bitcoin/src'
3make[2]: Entering directory '/store2/build/bitcoin/bitcoin/src'
4...
5 CXX test/test_test_bitcoin-raii_event_tests.o
6/.../bitcoin/src/test/raii_event_tests.cpp:5:10: fatal error: 'event2/event.h' file not found
7#include <event2/event.h>
8 ^~~~~~~~~~~~~~~~
91 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:
0/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
1.../bitcoin/src/test/raii_event_tests.cpp:5:10: fatal error: 'event2/event.h' file not found
2#include <event2/event.h>
3 ^~~~~~~~~~~~~~~~
41 error generated.
My configure script looks like:
0#!/bin/bash
1CLANGPATH=/opt/clang40
2$1/configure --with-incompatible-bdb \
3 --prefix=/tmp/bitcoin \
4 CC="${CLANGPATH}/bin/clang " CXX="${CLANGPATH}/bin/clang++" \
5 OBJCXX="${CLANGPATH}/bin/clang++" \
6 CPPFLAGS="-ggdb" \
7 EVENT_CFLAGS="-I/opt/libevent/include" \
8 EVENT_LIBS="-L/opt/libevent/lib -levent" \
9 EVENT_PTHREADS_CFLAGS="-I/opt/libevent/include" \
10 EVENT_PTHREADS_LIBS="-L/opt/libevent/lib -levent_pthreads" \
/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.