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.

Sign up for GitHub

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

Jump to bottom

Add string to time function #152

Merged
merged 33 commits into from
Sep 4, 2020
Merged

Add string to time function #152

merged 33 commits into from
Sep 4, 2020

Conversation

JShep1
Copy link

@JShep1 JShep1 commented Aug 28, 2020 •
edited
Loading

Adds a function converting a string of the general format "dd hh:mm:ss.nnn" where n is millisecond values to a std::chrono::steady_clock::time_point

Depends on #150

Signed-off-by: John Shepherd [email protected]

ahcorde and others added 9 commits August 26, 2020 11:17
@JShep1 JShep1 requested a review from ahcorde August 28, 2020 04:36
@JShep1 JShep1 requested a review from scpeters as a code owner August 28, 2020 04:36
John Shepherd added 3 commits August 27, 2020 21:39
Add string to time function
9f14978 
Signed-off-by: John Shepherd <[email protected]>
Merge branch 'jshep1/string_to_chrono_helper' of https://github.com/i…
81c0d78 
…gnitionrobotics/ign-math into jshep1/string_to_chrono_helper

Signed-off-by: John Shepherd <[email protected]>
Add tests
b04ca81 
Signed-off-by: John Shepherd <[email protected]>
@JShep1 JShep1 mentioned this pull request Aug 28, 2020
Merged
@JShep1 JShep1 added 📜 blueprint Ignition Blueprint enhancement New feature or request labels Aug 28, 2020
@codecov
Copy link

codecov bot commented Aug 28, 2020 •
edited
Loading

Codecov Report

Merging #152 into ign-math6 will decrease coverage by 0.01%.
The diff coverage is 97.72%.

Impacted file tree graph

@@              Coverage Diff              @@
##           ign-math6     #152      +/-   ##
=============================================
- Coverage      99.24%   99.23%   -0.02%     
=============================================
  Files             59       59              
  Lines           5823     5867      +44     
=============================================
+ Hits            5779     5822      +43     
- Misses            44       45       +1
Impacted Files Coverage Δ
include/ignition/math/Helpers.hh 98.75% <97.72%> (-0.39%) ⬇️

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 d5dc70f...65d145e. Read the comment docs.

John Shepherd and others added 3 commits August 27, 2020 22:49
ahcorde
ahcorde requested changes Aug 28, 2020
View reviewed changes
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Just a comment about try/catch

include/ignition/math/Helpers.hh Show resolved Hide resolved
John Shepherd and others added 3 commits August 28, 2020 10:23
Update result addition
be5708e 
Signed-off-by: John Shepherd <[email protected]>
change to auto
a866a24 
Signed-off-by: John Shepherd <[email protected]>
@scpeters
Fix macOS build
f11f678 
Cast to system_clock::duration where necessary.

Signed-off-by: Steve Peters <[email protected]>
ahcorde
ahcorde requested changes Aug 31, 2020
View reviewed changes
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

The compilation in mac is failing https://build.osrfoundation.org/job/ignition_math-ci-pr_any-homebrew-amd64/670/consoleFull#-9943238596ea37b8d-a6d4-40ae-8431-d90c018842af

@scpeters solved here the same proble on macos f11f678

@JShep1 JShep1 mentioned this pull request Sep 1, 2020
Merged
Update to steady clock
d7c1d31 
Signed-off-by: John Shepherd <[email protected]>
@JShep1 JShep1 requested a review from ahcorde September 2, 2020 00:28
@ahcorde
Added feedback
87d71f8 
Signed-off-by: ahcorde <[email protected]>
ahcorde
ahcorde approved these changes Sep 2, 2020
View reviewed changes
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

LGTM

ahcorde and others added 2 commits September 2, 2020 17:57
@ahcorde
alphabetize headers in helpers.hh
f9ef912 
Signed-off-by: ahcorde <[email protected]>
sync branch
c5cc779 
Signed-off-by: John Shepherd <[email protected]>
@chapulina chapulina added 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Sep 2, 2020
Base automatically changed from ahcorde/time/helper_functions to ign-math6 September 3, 2020 07:47
sync with math6
8fb84ee 
Signed-off-by: John Shepherd <[email protected]>
@JShep1 JShep1 mentioned this pull request Sep 3, 2020
Closed
fix windows warning
65d145e 
Signed-off-by: John Shepherd <[email protected]>
@chapulina
Copy link
Contributor

Not sure why codecov is marking some lines as uncovered, when they clearly are:

https://codecov.io/gh/ignitionrobotics/ign-math/pull/152/src/include/ignition/math/Helpers.hh?before=include/ignition/math/Helpers.hh

@JShep1
Copy link
Author

JShep1 commented Sep 3, 2020

Not sure why codecov is marking some lines as uncovered, when they clearly are:

https://codecov.io/gh/ignitionrobotics/ign-math/pull/152/src/include/ignition/math/Helpers.hh?before=include/ignition/math/Helpers.hh

I was also wondering this

@chapulina chapulina merged commit 87d3406 into ign-math6 Sep 4, 2020
@chapulina chapulina deleted the jshep1/string_to_chrono_helper branch September 4, 2020 20:37
@scpeters scpeters mentioned this pull request Sep 4, 2020
Merged
@scpeters
Copy link
Member

scpeters commented Sep 4, 2020

sdformat is building ign-math6 from source and is now reporting two windows warnings from code added in this PR:

scpeters
scpeters reviewed Sep 4, 2020
View reviewed changes
include/ignition/math/Helpers.hh
const std::string &_timeString)
{
std::chrono::steady_clock::time_point timePoint =
math::secNsecToTimePoint(-1, 0);
Copy link
Member

Choose a reason for hiding this comment

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

I'm seeing an msbuild warning when compiling this from source for sdformat:

'argument': conversion from 'int' to 'const uint64_t', signed/unsigned mismatch

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing it's complaining about the implicit cast of -1 to const uint64_t

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I just realized this, I guess an invalid string will have to return 0 now, I was using -1 as an indication of error, but now it seems that's not going to be possible

scpeters
scpeters reviewed Sep 4, 2020
View reviewed changes
include/ignition/math/Helpers.hh
{
numberDays = std::stoi(dayString);
}
catch (const std::out_of_range &oor)
Copy link
Member

Choose a reason for hiding this comment

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

another windows warning found by sdformat:

'oor': unreferenced local variable

Copy link
Member

Choose a reason for hiding this comment

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

I think this would be easy enough to fix by just deleting oor:

catch (const std::out_of_range &)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing this out, I'm creating a PR now

@JShep1 JShep1 mentioned this pull request Sep 4, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scpeters scpeters scpeters left review comments

@chapulina chapulina chapulina approved these changes

@ahcorde ahcorde ahcorde approved these changes

Assignees

@JShep1 JShep1

Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JShep1 @chapulina @scpeters @ahcorde