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 date time to trace (release 6.2) #4087

Conversation

sfc-gh-dyoungworth
Copy link
Collaborator

This PR is resolves #...

Changes in this PR:

General guideline:

  • If this PR is ready to be merged (and all checkboxes below are either ticked or not applicable), make this a regular PR
  • If this PR still needs work, please make this a draft PR
    • If you wish to get feedback/code-review, please add the label RFC to this PR

Please verify that all things listed below were considered and check them. If an item doesn't apply to this type of PR (for example a documentation change doesn't need to be performance tested), you should make a strikethrough (markdown syntax: ~~strikethrough~~). More infos on the guidlines can be found here.

Style

  • All variable and function names make sense.
  • The code is properly formatted (consider running git clang-format).

Performance

  • All CPU-hot paths are well optimized.
  • The proper containers are used (for example std::vector vs VectorRef).
  • There are no new known SlowTask traces.

Testing

  • The code was sufficiently tested in simulation.
  • If there are new parameters or knobs, different values are tested in simulation.
  • ASSERT, ASSERT_WE_THINK, and TEST macros are added in appropriate places.
  • Unit tests were added for new algorithms and data structure that make sense to unit-test
  • If this is a bugfix: there is a test that can easily reproduce the bug.

@sfc-gh-dyoungworth
Copy link
Collaborator Author

sfc-gh-dyoungworth commented Nov 19, 2020

This is the same change cherry-picked from my PR to master: #3985

Since there is no joshua for 6.2, I ran 10K correctness against the master branch with no errors.

Ideally this should go into 6.2, but I'm happy to push it to 6.3 if we think its not worth the risk

@@ -388,6 +389,7 @@ struct TraceEvent {
static bool isNetworkThread();

static double getCurrentTime();
static std::string printRealTime(double time);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: This doesn't need to be exposed in the header file.

@sfc-gh-mpilman sfc-gh-mpilman self-assigned this Nov 19, 2020
@sfc-gh-dyoungworth
Copy link
Collaborator Author

Also tested the 6.2 build locally to make sure the DateTime looked correct outside of simulation

@sfc-gh-mpilman sfc-gh-mpilman merged commit 67944f6 into apple:release-6.2 Nov 20, 2020
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.

None yet

3 participants