Skip to content

Deprecated common:Time#90

Merged
chapulina merged 17 commits intomasterfrom
ahcorde/std_chrono
Sep 18, 2020
Merged

Deprecated common:Time#90
chapulina merged 17 commits intomasterfrom
ahcorde/std_chrono

Conversation

@ahcorde
Copy link
Copy Markdown
Contributor

@ahcorde ahcorde commented Aug 25, 2020

This PR is part of this issue #61. The idea it's to deprecate the class common::Time in favor of std::chrono.

This two functions convert from time_point to seconds and nanoseconds and viceversa.

Signed-off-by: ahcorde ahcorde@gmail.com

@ahcorde ahcorde added enhancement New feature or request 🔮 dome Ignition Dome labels Aug 25, 2020
@ahcorde ahcorde requested a review from mjcarroll as a code owner August 25, 2020 18:09
@ahcorde ahcorde self-assigned this Aug 25, 2020
@github-actions github-actions bot added 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint labels Aug 25, 2020
This was referenced Aug 25, 2020
Merged
Merged
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 25, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@512f077). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #90   +/-   ##
=========================================
  Coverage          ?   73.78%           
=========================================
  Files             ?       68           
  Lines             ?     9209           
  Branches          ?        0           
=========================================
  Hits              ?     6795           
  Misses            ?     2414           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 512f077...6805de0. Read the comment docs.

@ahcorde ahcorde mentioned this pull request Aug 25, 2020
Closed
chapulina
chapulina suggested changes Aug 25, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I like the ideas on this PR, but I suggest we move the new functions to ign-math.

@ahcorde ahcorde force-pushed the ahcorde/std_chrono branch from cf46105 to 3d67a25 Compare August 26, 2020 07:17
@ahcorde ahcorde requested a review from maryaB-osr as a code owner August 26, 2020 07:17
@ahcorde ahcorde changed the base branch from ign-common3 to master August 26, 2020 07:17
@ahcorde
Deprecated Time constructors
a370f08 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde force-pushed the ahcorde/std_chrono branch from 3d67a25 to a370f08 Compare August 26, 2020 07:18
@ahcorde
Deprecated static functions
626c136 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde changed the title Added static function to handle std::chrono::time_point Deprecated common:Time Aug 26, 2020
@ahcorde
Copy link
Copy Markdown
Contributor Author

ahcorde commented Aug 26, 2020

  • Remove two new methods (moved to ign-math).
  • Deprecated constructor and static functions.
  • Retarget branch to master

This was referenced Aug 26, 2020
Merged
Merged
@ahcorde
use the right deprecation
9fccb68 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@chapulina chapulina removed 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Aug 31, 2020
chapulina
chapulina suggested changes Aug 31, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

We should resolve the introduced deprecation warnings before merging:

https://build.osrfoundation.org/job/ignition_common-ci-pr_any-ubuntu_auto-amd64/370/gcc/

Also, I think it would be a good idea to hold on merging this until we are sure that we can remove common::Time from all existing API on other libraries.

@ahcorde
Copy link
Copy Markdown
Contributor Author

ahcorde commented Sep 2, 2020

@chapulina, This issue is the meta-ticket to follow all the libraries that currently are using common::Time and it points to the related PRs to replace the use of common::Time with std::chrono

@ahcorde
Replaced common::Time with std::chrono in Timer class
6f96981 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Avoid deprecation warning
72b55b4 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from chapulina September 2, 2020 13:40
@ahcorde
fixed action
f253e4d 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
fixed action
c798f50 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Copy Markdown
Contributor Author

ahcorde commented Sep 2, 2020

I fixed the after_make.sh script in this PR, but I think the right solution is in this other PR gazebo-tooling/action-gz-ci#22

chapulina
chapulina suggested changes Sep 8, 2020
View reviewed changes
@ahcorde
Added WaitForResults using std::chrono and updated workerpool test
8377345 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
removed deprecation warning in timer class
56184b2 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Restored and deprecated Elapsed in timer class and create a ElapseTim…
3bffc54 
…e method

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
make linters happy
b673886 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from chapulina September 16, 2020 07:41
chapulina
chapulina reviewed Sep 17, 2020
View reviewed changes
@ahcorde
Timer ElapsedTime method returns a std::chrono::duration
0d648d4 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
avoid duplicating the logic in WorkerPool::WaitForResults
3d21cb5 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde requested a review from chapulina September 17, 2020 08:07
@ahcorde
Copy link
Copy Markdown
Contributor Author

ahcorde commented Sep 17, 2020

@osrf-jenkins run tests

@chapulina
bump ign-math version
58a9096 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
chapulina
chapulina approved these changes Sep 18, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM with happy CI. I just have one minor comment about code duplication.

@ahcorde
Ignored warning DLL interface missing on windows
5d22e97 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Merge branch 'ahcorde/std_chrono' of https://github.com/ignitionrobot…
3531354 
…ics/ign-common into ahcorde/std_chrono
@ahcorde
Avoided duplicated code
6805de0 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Copy Markdown
Contributor Author

ahcorde commented Sep 18, 2020

@osrf-jenkins run tests

@chapulina chapulina merged commit 526c2ac into master Sep 18, 2020
@chapulina chapulina deleted the ahcorde/std_chrono branch September 18, 2020 16:50
@chapulina chapulina mentioned this pull request Jul 20, 2022
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iche033 iche033 iche033 left review comments

@mjcarroll mjcarroll mjcarroll approved these changes

@maryaB-osr maryaB-osr Awaiting requested review from maryaB-osr

+1 more reviewer

@chapulina chapulina chapulina approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ahcorde ahcorde

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ahcorde @mjcarroll @iche033 @chapulina @j-rivero @osrf-triage