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

include/assert: clean up ceph assertion macros #9969

Merged
merged 5 commits into from Jul 4, 2016

Conversation

liewegas
Copy link
Member

No description provided.

Signed-off-by: Sage Weil <sage@redhat.com>
Abort with an error string.

Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
We will transition to these.

Signed-off-by: Sage Weil <sage@redhat.com>
These variants we promise never to compile out based on NDEBUG.

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
Copy link
Member Author

test this please

@@ -129,6 +129,13 @@ using namespace ceph;
? __CEPH_ASSERT_VOID_CAST (0) \
: __ceph_assert_fail (__STRING(expr), __FILE__, __LINE__, __CEPH_ASSERT_FUNCTION))

// this variant will *never* get compiled out to NDEBUG in the future.
// (ceph_assert currently doesn't either, but in the future it might.)
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 not very familiar with how our current asserts work with debug that lets them be disabled or not. Can we do something to make sure the tooling is set up properly right now, which guarantees ceph_assert_always really does get compiled in?

Copy link
Member Author

@liewegas liewegas Jun 29, 2016

Choose a reason for hiding this comment

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

Ours aren't conditional (yet)... It's the lack of an #ifdef NDEBUG where this comment goes that prevents it from compiling out. And/or the lack of a similar comment says "do not ever compile this out" next to the ceph_assert definition a few lines up.

@gregsfortytwo
Copy link
Member

Basically looks fine.

@tchaikov tchaikov merged commit c5d50ca into ceph:master Jul 4, 2016
@liewegas liewegas deleted the wip-assert branch August 12, 2016 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants