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

First step of simplifying and improving frames #2399

Merged
merged 8 commits into from
Dec 12, 2019

Conversation

pleroy
Copy link
Member

@pleroy pleroy commented Dec 11, 2019

  • Make it possible to create a frame using a local enum class.
  • Replace the boolean is_inertial by an enum.
  • Add handedness (not used for now).

//
// or:
// enum class MyFrameTag;
// using MyFrame = Frame<MyFrameTag, MyFrameTag{}, Inertial>;
Copy link
Member

Choose a reason for hiding this comment

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

  1. Frame<enum class MyFrameTag, MyFrameTag{}, Inertial>; would work (do it that way in the other files);
  2. We should put the tag_ parameter last, so that it can be omitted even if inertia and handedness are provided: when we define a formal namespace-scope serializable frame, having to explicitly specify inertia and handedness is a feature, and for a local throwaway frame with a local enum class, the tag_ is the most useless parameter (and thus should be the most defaulted one).

Item 2. should probably be done in a separate pull request, it will be a little bit noisy.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Done.
  2. Let's wait until the dust settles.

@@ -16,12 +16,41 @@ namespace internal_frame {
using base::not_constructible;
using base::not_null;

template<typename FrameTag, FrameTag frame_tag, bool frame_is_inertial>
enum Inertia {
Copy link
Member

Choose a reason for hiding this comment

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

ReferenceFrameInertia; we don't see that name much because it's an enum rather than an enum class, so it doesn't need to be catchy, and using up Inertia saddens me (see my comment about the possibility of renaming InertiaTensor in a recent pull request).

Even better, ReferenceFrameMotion: I was thinking of distinguishing rotating and non-rotating inertial frames at some point for KeplerOrbit.

Sorry, something went wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

The class is called Frame, not ReferenceFrame, so the word "reference" is weird. Going for FrameMotion.

Sorry, something went wrong.

@@ -28,7 +44,7 @@ TEST_F(FrameDeathTest, SerializationError) {
serialization::Frame message;
World1::WriteToMessage(&message);
World2::ReadFromMessage(message);
}, "tag ==");
}, "\\(tag\\) ==");
Copy link
Member

Choose a reason for hiding this comment

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

R"(\(tag\) ==)"

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, but I'm not sure if the extra parentheses are better than the extra \.

@eggrobin eggrobin added the LGTM label Dec 12, 2019
@pleroy pleroy merged commit 1abbaa5 into mockingbirdnest:master Dec 12, 2019
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.

None yet

2 participants