-
-
Notifications
You must be signed in to change notification settings - Fork 510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
294 ios backend framework #299
Conversation
#RENAME libtinyxml2.dylib | ||
) | ||
endif() | ||
if(NOT IOS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could just write if (APPLE AND NOT IOS)
, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, because there is an else()
at the end of the tinyxml2 block, that would then take linux and iOS :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're right.
BUILD_WITH_INSTALL_RPATH TRUE | ||
INSTALL_NAME_DIR @rpath | ||
MACOSX_FRAMEWORK_IDENTIFIER io.dronecore.backend | ||
VERSION 0.0.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can get the version later from the latest git tag.
grpc/server/src/backend.cpp
Outdated
|
||
dronecore::DroneCore _dc; | ||
dronecore::backend::ConnectionInitiator<dronecore::DroneCore> _connection_initiator; | ||
std::unique_ptr<grpc::Server> server; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not _server
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
argh. I lost it in the rebase. And I actually verified that it was still _server
xD. Will change, sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I actually messed up, because it fails the test. Just the CI is not compiling grpc for now :D.
grpc/server/src/backend.h
Outdated
DroneCoreBackend(); | ||
~DroneCoreBackend(); | ||
|
||
DroneCoreBackend(DroneCoreBackend &&op) noexcept; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the noexcept
needed? Shouldn't everything be compiled without exception anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not, sure, I copied a pImpl example to not mess it up. Should I remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess so, not sure. Probably not important.
8e96747
to
cba697d
Compare
cba697d
to
8c567c7
Compare
|
||
DroneCoreBackend::DroneCoreBackend() : _impl(new Impl()) {} | ||
DroneCoreBackend::~DroneCoreBackend() = default; | ||
DroneCoreBackend::DroneCoreBackend(DroneCoreBackend &&) = default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does default
mean for these? What about using delete
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This time, I think it is explained in the article showing the pImpl coming from Scott Meyers' book (article here. Specifically, it says:
// Tell the compiler to generate default special members which utilize
// the power of std::unique_ptr.
// We can do it here because the implementation class is defined at this point thus
// std::unique_ptr can properly handle the implementation pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can still change it later if you want, but as far as I understand, default
is not wrong. So I'll already merge that :-).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's continue this discussion here.
When building for iOS, the backend is now automatically built as an iOS Framework that can be directly added in an Xcode project and used (it only exposes the one
runBackend(port)
function).There are two small glitches regarding dependencies:
-DBUILD_STATIC_LIBS
) that we may not want to set for the whole project. So I'd like to build tinyxml2 separately (like grpc and others). For now, I don't link it in iOS.Fixes #294.