Minor fixes to enable building with clang on OS X #56
Conversation
|
Revised commit only changed the commit comment to reference the issue. |
| @@ -5,7 +5,7 @@ | |||
| http://www.boost.org/LICENSE_1_0.txt) | |||
| ------------------------------------------------------------------------------*/ | |||
|
|
|||
| #include "endian.h" | |||
| #include "endian.hpp" | |||
There was a problem hiding this comment.
#include "endian.hpp" should actually be removed entirely. It's endian.hpp that includes endian.ipp, and not the other way around.
| @@ -20,6 +20,7 @@ | |||
| #include "connector.hpp" | |||
| #include "error.hpp" | |||
| #include "rawsockdefs.hpp" | |||
| #include "./internal/udsopener.hpp" | |||
There was a problem hiding this comment.
This breaks the Pimpl idiom intended by
std::unique_ptr<Impl> impl_;
You didn't do the same in tcpconnector.hpp. Was there a clang error that made you do this? If so, what was the error?
There was a problem hiding this comment.
You're right. I didn't notice the parallel with tcpconnector.hpp. It's due to some inconsistency in how clang handles nested classes for forward declarations. I'll have a better fix shortly.
|
I checked everything except for the tuple stuff. I'm a bit too tired to parse that recursive madness, lol. I'll try going over the tuple stuff tomorrow evening. Most of it looks fine, and I left comments where I had concerns / questions. Please respond when you get the chance. I see that you already addressed one of them. We might need more test cases involving |
taion
force-pushed
the
jjia/clang-build
branch
2 times, most recently
from
0e47df3
to
ab43b70
Compare
|
Accidentally checked in the wrong code for the last commit a couple of times. This last version keeps the correct forwarding semantics on the constructor arguments, and correctly adds the test case to ensure that converting a nested tuple into a nested array still works. |
| // that for some reason: http://stackoverflow.com/q/24454664. | ||
| template <std::size_t N, typename... Ts> | ||
| using EnableIfTupleElement = | ||
| typename std::enable_if<N != sizeof...(Ts), int>::type; |
There was a problem hiding this comment.
This is actually cleaner than what I had before. Kudos!
|
Thanks! |
|
Good job, and thanks again for the contribution! 👍 Everything still work fine on my compiler/platform with no warnings. Units tests are ok. Do you mind telling me which operating systems you tested this on, so that I can add a few more supported platforms to the project's README? You've already mentioned the compiler versions above. |
|
OS X 10.10.3 (with clang) I haven't actually built anything using the library yet. I only ran the tests, so I'm not positive everything works when actually using the library, but I plan to get to that in the next week or so. |
|
Thanks. The README says: "This library has been tested with: ...", so its not making any bold claims, hehe. |
There's 3 things going on here.
TupleResultandTupleLastResultnot properly compiling in clang, but with a workaround that I think is good enough.std::vector<bool>specialization in Apple's libc++ that's even worse than the normalstd::vector<bool>specialization: https://llvm.org/bugs/show_bug.cgi?id=10314Built and tested with Xcode 6.3.1 toolchain and gcc 4.9.2.
ETA: For #55 (adding this to get the link to the PR from the issue)