Skip to content

Modifications and cleanup for compilation on OS/X#42

Closed
waTeim wants to merge 4 commits intoetr:masterfrom
waTeim:master
Closed

Modifications and cleanup for compilation on OS/X#42
waTeim wants to merge 4 commits intoetr:masterfrom
waTeim:master

Conversation

@waTeim
Copy link

@waTeim waTeim commented Feb 18, 2014

I've made modifications mostly for compilation on OS/X to succeed. Found 1 generic bug and one bug that is OS/X specific. The resulting library have been tested by inclusion in a program compiled for both OS/X and Linux. The changes are limited in scope and can be verified readily by inspection. I also made some changes that are not strictly necessary, but suited to my development environment.

More specifically;

  1. clang requires that the copy constructor of http_endpoint be public.

  2. Changed to match declarations and forward declarations; class for class, struct for struct

  3. clang may have further requirements needing fully defined types or there is a scenario not seen before that can give rise to use of a class not fully defined for which include file inclusion fixes the problem

  4. OS/X AF_INET STREAM_SOCKETS do not support SOCK_CLOSEXEC; the system call returns EPROTONOSUPPORT not EINVAL.

  5. Changed a getter to const as it doesn't change the class state and expands the scenarios in which a temporary object need not be created.

  6. Fixed a typo in set_requestor_port.

  7. Changed to allow building in the same directory, modified .gitignore to ignore the resulting targets

Added various include directives in http_response.cpp and
http_resource.cpp and modified http_endpoint copy constructor to
make it public to satifify clang.

Fixed typo/bug in http_request::set_requestor_port.

Modified various forwards to match actual declarations.

Modified configure to allow build in same directory.
@etr
Copy link
Owner

etr commented Feb 18, 2014

Thank you for your help. I discuss here the changes. I am a lot interested in having details especially regarding the points 1 and 3.

  1. Point one is unclear - it is a specific design choice for that constructor to not be public. I would like to understand why clang need it to be public to see if it is possible to mediate the solution.

  2. Perfect.

  3. As per point 1. It is weird that clang do not tolerate standard use of implementation hiding. I am actually moving forward in the future to press much more in hiding some parts of the implementation inside "details" package.

  4. Ok

  5. Perfect.

  6. Ok

  7. Not ok on building in the same directory - do not understand the reason behind this change.

@waTeim
Copy link
Author

waTeim commented Feb 18, 2014

  1. and 3) I think this may be the difference between what gcc and clang deem to be appropriate circumstances necessary for copy elision. In that circumstance gcc elided the constructor -- is is trivially copyable? -- and clang did not, and thus needed the copy constructor to be accessible. It's just a guess though. What I can do is undo the changes privately and report back the error message and locations where clang complains.

  2. Yea I figured, especially since you put in a change to specifically prevent this. All I can say is as much as you don't like it being that way I don't like it not being that way. It's your project, do I'll go with the flow and keep it only on my fork. I hope there's an easy way for each of us to keep this separate. If you need me to do something special make it easy for you to maintain what you want let me know.

@etr
Copy link
Owner

etr commented Feb 18, 2014

Don't worry, I will probably anyway move the pull request manually.

I installed a clang instance to test it and see if I can obtain something that actually works fine with both compilers.

Regarding 7) yes, I really don't like compilations in the same folder - they usually end up in mixups between code and binaries that more than once have made me cry blood to be solved.

Once I move the pull request (or part of it) inside the code, I will add you in the AUTHORS file as a collaborator in the format:

FirstName FamilyName emailAddress

let me know if for any reason you don't want to have your name or your e-mail address inserted there.

@waTeim
Copy link
Author

waTeim commented Feb 18, 2014

Cool!

Name: Jeff Waller

Also, just to make it fun, XCode 5 clang is not the latest clang...

clang --version
Apple LLVM version 5.0 (clang-500.2.79) (based on LLVM 3.3svn)
Target: x86_64-apple-darwin13.0.2
Thread model: posix

  1. is easy anyway. If http_endpoint copy constructor is private, the following results, so maybe it's Apple's or clang's implementation of std:?

In file included from webserver.cpp:23:

...

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:1505:36: error: calling a private constructor of class 'httpserver::details::http_endpoint'
::new ((void*)__p) _Tp(__a0);
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/map:1258:20: note: in instantiation of function template specialization
'std::__1::allocator_traits<std::__1::allocator<std::__1::__tree_node<std::__1::pair<httpserver::details::http_endpoint, httpserver::details::http_resource_mirror>, void *> >
>::construct<httpserver::details::http_endpoint, httpserver::details::http_endpoint>' requested here
__node_traits::construct(__na, _VSTD::addressof(__h->_value.first), __k);
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/map:1276:29: note: in instantiation of member function 'std::__1::map<httpserver::details::http_endpoint,
httpserver::details::http_resource_mirror, std::__1::lesshttpserver::details::http_endpoint, std::__1::allocator<std::__1::pair<const httpserver::details::http_endpoint,
httpserver::details::http_resource_mirror> > >::__construct_node' requested here
__node_holder __h = __construct_node(__k);
^
webserver.cpp:255:71: note: in instantiation of member function 'std::__1::map<httpserver::details::http_endpoint, httpserver::details::http_resource_mirror, std::__1::lesshttpserver::details::http_endpoint,
std::__1::allocator<std::__1::pair<const httpserver::details::http_endpoint, httpserver::details::http_resource_mirror> > >::operator[]' requested here
registered_resources_str[idx.url_complete] = &registered_resources[idx];
^
./httpserver/details/http_endpoint.hpp:67:9: note: implicitly declared private here
http_endpoint(const http_endpoint& h);

@etr
Copy link
Owner

etr commented Feb 22, 2014

I made some changes. It seems to compile on both my clang 3.2 installation and clang 3.3 on travis-ci. Let me know if the current trunk works on your clang 5.0.
I also created an option in configure "--enable-same-directory-build" that disable the control (this way you can avoid to fork and have to align my changes back).

@waTeim
Copy link
Author

waTeim commented Feb 23, 2014

Hey I like the configure option!

Unfortunately, I think OS/X std impl is going to insist that http_endpoint is publicly available it's essentially the same error as before.

Which appears to arise from this one statement:

registered_resources_str[idx.url_complete] = &registered_resources[idx];

Makes sense too because of this:

webserver.hpp:232

std::map<details::http_endpoint, details::http_resource_mirror> registered_resources;

if instead it was something like

std::map<details::http_endpoint, details::http_resource_mirror*> registered_resources;

or

std::map< details::http_endpoint, std::shared_ptr<details::http_resource_mirror> > registered_resources;

this second option is probably going to force std=c++11 though, which is limiting, but maybe no big deal in 2014.

that would probably satisfy the OS/X impl.

mv -f .deps/string_utilities.Tpo .deps/string_utilities.Plo
/bin/sh ../libtool --tag=CXX --mode=compile clang++ -DHAVE_CONFIG_H -I. -I.. -I../ -I./httpserver/ -O3 -fPIC -Wall -DHTTPSERVER_COMPILATION -D_REENTRANT -I/usr/local/include -MT webserver.lo -MD -MP -MF .deps/webserver.Tpo -c -o webserver.lo webserver.cpp
libtool: compile: clang++ -DHAVE_CONFIG_H -I. -I.. -I../ -I./httpserver/ -O3 -fPIC -Wall -DHTTPSERVER_COMPILATION -D_REENTRANT -I/usr/local/include -MT webserver.lo -MD -MP -MF .deps/webserver.Tpo -c webserver.cpp -fno-common -DPIC -o .libs/webserver.o
In file included from webserver.cpp:23:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/iostream:38:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/ios:216:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/_locale:15:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/string:434:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/algorithm:594:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:1505:36: error: calling a private constructor of class
'httpserver::details::http_endpoint'
::new ((void
)__p) _Tp(__a0);
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/map:1258:20: note: in instantiation of function template specialization
'std::__1::allocator_traits<std::__1::allocator<std::__1::__tree_node<std::__1::pair<httpserver::details::http_endpoint, httpserver::details::http_resource_mirror>, void *> >
>::construct<httpserver::details::http_endpoint, httpserver::details::http_endpoint>' requested here
__node_traits::construct(__na, _VSTD::addressof(__h->_value.first), __k);
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/map:1276:29: note: in instantiation of member function
'std::__1::map<httpserver::details::http_endpoint, httpserver::details::http_resource_mirror, std::__1::lesshttpserver::details::http_endpoint, std::__1::allocator<std::__1::pair<const
httpserver::details::http_endpoint, httpserver::details::http_resource_mirror> > >::__construct_node' requested here
__node_holder __h = __construct_node(__k);
^
webserver.cpp:255:71: note: in instantiation of member function 'std::__1::map<httpserver::details::http_endpoint, httpserver::details::http_resource_mirror,
std::__1::lesshttpserver::details::http_endpoint, std::__1::allocator<std::__1::pair<const httpserver::details::http_endpoint, httpserver::details::http_resource_mirror> > >::operator[]'
requested here
registered_resources_str[idx.url_complete] = &registered_resources[idx];
^
./httpserver/details/http_endpoint.hpp:68:9: note: declared private here
http_endpoint(const http_endpoint& h);
^
In file included from webserver.cpp:23:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/iostream:38:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/ios:216:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/__locale:15:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/string:434:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/algorithm:594:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:1628:23: error: 'http_endpoint' is a private member of
'httpserver::details::http_endpoint'
__p->
_Tp();
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:1526:14: note: in instantiation of function template specialization
'std::__1::allocator_traits<std::__1::allocator<std::__1::__tree_node<std::__1::pair<httpserver::details::http_endpoint, httpserver::details::http_resource_mirror>, void *> >
>::__destroyhttpserver::details::http_endpoint' requested here
{__destroy(__has_destroy<allocator_type, Tp>(), __a, __p);}
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/map:526:29: note: in instantiation of function template specialization
'std::__1::allocator_traits<std::__1::allocator<std::__1::__tree_node<std::__1::pair<httpserver::details::http_endpoint, httpserver::details::http_resource_mirror>, void *> >
>::destroyhttpserver::details::http_endpoint' requested here
__alloc_traits::destroy(_na, _VSTD::addressof(__p->_value.first));
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2687:13: note: in instantiation of member function
'std::__1::__map_node_destructor<std::__1::allocator<std::__1::__tree_node<std::__1::pair<httpserver::details::http_endpoint, httpserver::details::http_resource_mirror>, void *> >
>::operator()' requested here
_ptr.second()(__tmp);
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2655:46: note: in instantiation of member function
'std::__1::unique_ptr<std::__1::__tree_node<std::__1::pair<httpserver::details::http_endpoint, httpserver::details::http_resource_mirror>, void *>,
std::__1::__map_node_destructor<std::__1::allocator<std::__1::__tree_node<std::__1::pair<httpserver::details::http_endpoint, httpserver::details::http_resource_mirror>, void *> > > >::reset'
requested here
_LIBCPP_INLINE_VISIBILITY ~unique_ptr() {reset();}
^
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/map:1276:29: note: in instantiation of member function
'std::__1::unique_ptr<std::__1::__tree_node<std::__1::pair<httpserver::details::http_endpoint, httpserver::details::http_resource_mirror>, void *>,
std::__1::__map_node_destructor<std::__1::allocator<std::__1::__tree_node<std::__1::pair<httpserver::details::http_endpoint, httpserver::details::http_resource_mirror>, void *> > >
>::~unique_ptr' requested here
__node_holder __h = __construct_node(__k);
^
webserver.cpp:255:71: note: in instantiation of member function 'std::__1::map<httpserver::details::http_endpoint, httpserver::details::http_resource_mirror,
std::__1::lesshttpserver::details::http_endpoint, std::__1::allocator<std::__1::pair<const httpserver::details::http_endpoint, httpserver::details::http_resource_mirror> > >::operator[]'
requested here
registered_resources_str[idx.url_complete] = &registered_resources[idx];
^
./httpserver/details/http_endpoint.hpp:72:9: note: declared private here
~http_endpoint(); //if inlined it causes problems during ruby wrapper compiling

@etr
Copy link
Owner

etr commented Feb 23, 2014

I think they are avoiding somehow the type erasure in new versions - since it compiles with 3.2 and 3.3.
A pointer or shared_ptr in the value type would not solve the problem since what clang claims to want be public is the key not the value.
I will try to install a clang 5.

@waTeim
Copy link
Author

waTeim commented Feb 23, 2014

Oops, haha, you're right of course, but then same comments, but for the key not the value.

@etr
Copy link
Owner

etr commented Feb 24, 2014

I tried to block the possibilities for clang to build the partial object. I do not see why clang need to build a temporary copy of the key while inserting - this actually does not happen in linux implementation of LLVM 3.3 that matches your version on Mac OS/X. This make me strongly suspect that Mac implementation is just wrong.

It would be long to explain the details of why I need an object and not a pointer as key and it is much more easy to explain why it is not possible to use C++11 costructs since there are guys compiling this library with gcc 4.1.

I still think that having a public constructor for http_endpoint is not a good choice and I will not change this because they are not able to build a decent compiler.

Said this, let me know if this new version works, otherwise I will add an #ifdef that moves to public the constructor in OS/X environments - at least this shame will be confined.

@etr etr closed this Apr 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants