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 functions for C++ assertions #31

Merged
merged 9 commits into from
Feb 6, 2020
Merged

Add functions for C++ assertions #31

merged 9 commits into from
Feb 6, 2020

Conversation

zmichaels11
Copy link
Contributor

@zmichaels11 zmichaels11 commented Jan 23, 2020 •
edited
Loading

Changes

  • Add rcpputils::require_true(condition) which throws std::invalid_argument if condition is false. Its indended use is for parameter checks that should always be ran.
  • Add rcpputils::check_true(condition) which throws rcpputils::InvalidStateException if condition is false. Its intended use is for state checks that should always be ran.
  • Add rcpputils::assert_true(condition) which throws rcpputils::AssertionException if condition is false AND the macroNDEBUG is not set. Its intended use is for additional checks inserted into debug builds.

Issues

@zmichaels11 zmichaels11 requested a review from a team as a code owner January 23, 2020 19:04
@zmichaels11
Add functions for C++ assertions
4211b65 
Signed-off-by: Zachary Michaels <[email protected]>
@zmichaels11 zmichaels11 mentioned this pull request Jan 23, 2020
Closed
piraka9011
piraka9011 reviewed Jan 23, 2020
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
@piraka9011
Copy link

This LGTM, main concern is whether this works on windows.

emersonknapp
emersonknapp approved these changes Jan 23, 2020
View reviewed changes
Copy link
Contributor

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

LGTM pending green CI

@zmichaels11
Copy link
Contributor Author

@ros2/aws-oncall @prajakta-gokhale @thomas-moulard - please run this CI job
Gist: https://gist.githubusercontent.com/zmichaels11/9af526e5a269ea6e1f4d76afa1379aeb/raw/9f8084b3b84736076bea5cdde99b30576154006a/ros2.repos
BUILD args: --packages-up-to rcpputils
TEST args: --packages-select rcpputils
Job: ci_launcher

@zmichaels11
Copy link
Contributor Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@zmichaels11
Copy link
Contributor Author

A few changes, some from discussion with @wjwwood

  • renamed ros_assert to assert_true and check to check_true
  • use target_compile_definitions to specify NDEBUG in test_assert_ndebug.
  • add AssertionException and IllegalStateException for assert_true and check_true respectively.
  • add require_true which throws an std::invalid_argument

piraka9011
piraka9011 approved these changes Jan 23, 2020
View reviewed changes
Copy link

@piraka9011 piraka9011 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM on Green CI

wjwwood
wjwwood approved these changes Jan 23, 2020
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

Other than a small comment, lgtm

CMakeLists.txt Show resolved Hide resolved
@zmichaels11
Copy link
Contributor Author

Running CI with changes:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@zmichaels11
Copy link
Contributor Author

Rerun Windows CI

  • Build Status

@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Jan 23, 2020 •
edited
Loading

  • Windows Build Status
    Edit:
    reran due to disabling wrong warning

@zmichaels11
Copy link
Contributor Author

  • Windows Build Status

@zmichaels11
Copy link
Contributor Author

Ok I think I got it this time.

  • Windows Build Status

@zmichaels11
Copy link
Contributor Author

Does this require CI to rerun for Linux/macOS?

@piraka9011
Copy link

Does this require CI to rerun for Linux/macOS?

Better safe than sorry :)

@zmichaels11
Copy link
Contributor Author

zmichaels11 commented Jan 24, 2020 •
edited
Loading

Rest of CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status

@emersonknapp emersonknapp merged commit 3eededd into ros2:master Feb 6, 2020
@zmichaels11 zmichaels11 deleted the zmichaels11/asserts branch February 6, 2020 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piraka9011 piraka9011 piraka9011 approved these changes

@emersonknapp emersonknapp emersonknapp approved these changes

@wjwwood wjwwood wjwwood approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zmichaels11 @piraka9011 @wjwwood @emersonknapp