Skip to content
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

Add support for transmitting the status's message to the C# code #2588

Merged
merged 11 commits into from
May 23, 2020

Conversation

pleroy
Copy link
Member

@pleroy pleroy commented May 21, 2020

Changed the flight planner to output the error and message in case of unexpected error, and verified that the status is correctly propagated to the game.

As usual there is a commit before the change and a commit after the change for the generated files.

Fix #2583.
Contributes to #2585.

@pleroy pleroy modified the milestone: Galileo May 21, 2020
@@ -155,7 +156,8 @@ KeplerianElements ToKeplerianElements(
QP ToQP(DegreesOfFreedom<World> const& dof);
QP ToQP(RelativeDegreesOfFreedom<AliceSun> const& relative_dof);

Status ToStatus(base::Status const& status);
Status const* ToStatus(base::Status const& status);
Status const* ToStatus(base::Error error, std::string const& message);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ownership of the result is given to the caller.
Since these are not interface functions directly, we should document this.
On the other hand, since these are not interface functions, we have a way of documenting this: return std::unique_ptr, and call ToStatus(status).release() in the actual interface function.

Copy link
Member

@eggrobin eggrobin May 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return std::unique_ptr

Hm, that does not work since Status is not RAII because of its C string.

Document that ownership is returned, and call it MakeStatus or NewStatus.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ToNewStatus.

@@ -553,7 +553,7 @@ void JournalProtoProcessor::ProcessRequiredMessageField(
}
field_cxx_mode_fn_[descriptor] =
[](std::string const& type) {
return type + "*";
return type + " const*";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? It feels odd to return ownership to the caller, but not mutability.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, done.

@eggrobin eggrobin added the LGTM label May 22, 2020
@pleroy pleroy merged commit 54b64f7 into mockingbirdnest:master May 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The flight plan should surface unexpected statuses
2 participants