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

Fix more unused variable warnings #1059

Merged
merged 1 commit into from Jun 3, 2014

Conversation

Projects
None yet
3 participants
@mgaunard
Contributor

mgaunard commented Feb 2, 2014

Those changes prevent more unused variable warnings to occur, in particular when building with -DNDEBUG, which disables asserts.

@@ -16,23 +16,35 @@
namespace hpx { namespace util { namespace detail
{
template <typename Lock>
void assert_owns_lock(Lock& l, int)

This comment has been minimized.

@hkaiser

hkaiser Feb 2, 2014

Member

This file could be patched more easily: if HPX_DEBUG is not defined (which controls what HPX_ASSERT does) you could leave just the

template <typename Lock>
void assert_owns_lock(Lock&, int) {}

while not touching the code if HPX_DEBUG is defined.

@hkaiser

hkaiser Feb 2, 2014

Member

This file could be patched more easily: if HPX_DEBUG is not defined (which controls what HPX_ASSERT does) you could leave just the

template <typename Lock>
void assert_owns_lock(Lock&, int) {}

while not touching the code if HPX_DEBUG is defined.

This comment has been minimized.

@mgaunard

mgaunard Feb 2, 2014

Contributor

HPX_DEBUG doesn't actually control what HPX_ASSERT does.

defined(HPX_DISABLE_ASSERTS) || defined(BOOST_DISABLE_ASSERTS) || defined(NDEBUG) is the only way to check whether HPX_ASSERT does anything or not.

@mgaunard

mgaunard Feb 2, 2014

Contributor

HPX_DEBUG doesn't actually control what HPX_ASSERT does.

defined(HPX_DISABLE_ASSERTS) || defined(BOOST_DISABLE_ASSERTS) || defined(NDEBUG) is the only way to check whether HPX_ASSERT does anything or not.

This comment has been minimized.

@hkaiser

hkaiser Feb 2, 2014

Member

Sure, the main thing I wanted to suggest is to do the refactoring in a different way, namely fully separate the two branches, one for assertions off and one for assertions on. It does not matter which preprocessor expression is used to distinguish the two.

@hkaiser

hkaiser Feb 2, 2014

Member

Sure, the main thing I wanted to suggest is to do the refactoring in a different way, namely fully separate the two branches, one for assertions off and one for assertions on. It does not matter which preprocessor expression is used to distinguish the two.

@@ -55,7 +55,7 @@
return static_cast<char>(number_tmp - 10 + 'A');
}
#if defined(HPX_DISABLE_ASSERTS)
#if defined(HPX_DISABLE_ASSERTS) || defined(BOOST_DISABLE_ASSERTS) || defined(NDEBUG)

This comment has been minimized.

@hkaiser

hkaiser Feb 2, 2014

Member

We don't use NDEBUG anywhere in the code base, IIRC. It would be more consistent to use !defined(HPX_DEBUG) instead.

@hkaiser

hkaiser Feb 2, 2014

Member

We don't use NDEBUG anywhere in the code base, IIRC. It would be more consistent to use !defined(HPX_DEBUG) instead.

This comment has been minimized.

@mgaunard

mgaunard Feb 2, 2014

Contributor

you use standard assert, which uses NDEBUG.

@mgaunard

mgaunard Feb 2, 2014

Contributor

you use standard assert, which uses NDEBUG.

This comment has been minimized.

@hkaiser

hkaiser Feb 2, 2014

Member

That's a fair point and I was not aware we use std assert(), still. However, I'd rather switch those occurrences over to HPX_ASSERT than to introduce reliance on NDEBUG (mainly because NDEBUG is a platform specific macro).

@hkaiser

hkaiser Feb 2, 2014

Member

That's a fair point and I was not aware we use std assert(), still. However, I'd rather switch those occurrences over to HPX_ASSERT than to introduce reliance on NDEBUG (mainly because NDEBUG is a platform specific macro).

This comment has been minimized.

@mgaunard

mgaunard Feb 2, 2014

Contributor

NDEBUG is not a platform-specific macro. assert is a standard macro, and the standard says that it does nothing if NDEBUG is defined at the point of inclusion of assert.h/cassert.

What could be done, however, is to define a single HPX-specific macro if assertions are disabled, regardless of the method used. I'll try to make a suggestion about this.

@mgaunard

mgaunard Feb 2, 2014

Contributor

NDEBUG is not a platform-specific macro. assert is a standard macro, and the standard says that it does nothing if NDEBUG is defined at the point of inclusion of assert.h/cassert.

What could be done, however, is to define a single HPX-specific macro if assertions are disabled, regardless of the method used. I'll try to make a suggestion about this.

This comment has been minimized.

@hkaiser

hkaiser Feb 2, 2014

Member

Ok, I was not aware of NDEBUG being in the Standard. Anyways - thanks for looking into this!

@hkaiser

hkaiser Feb 2, 2014

Member

Ok, I was not aware of NDEBUG being in the Standard. Anyways - thanks for looking into this!

@ghost ghost assigned hkaiser Feb 2, 2014

@hkaiser hkaiser closed this Mar 25, 2014

@hkaiser hkaiser reopened this Mar 25, 2014

@hkaiser hkaiser modified the milestones: 0.9.9, 0.9.8 Mar 25, 2014

@StellarBot

This comment has been minimized.

Show comment
Hide comment
@StellarBot

StellarBot Apr 30, 2014

Contributor

Can one of the admins verify this patch?

Contributor

StellarBot commented Apr 30, 2014

Can one of the admins verify this patch?

@StellarBot

This comment has been minimized.

Show comment
Hide comment
@StellarBot

StellarBot May 26, 2014

Contributor

Can one of the admins verify this patch?

Contributor

StellarBot commented May 26, 2014

Can one of the admins verify this patch?

@hkaiser hkaiser merged commit 013851f into STEllAR-GROUP:master Jun 3, 2014

hkaiser added a commit that referenced this pull request Jun 3, 2014

@hkaiser

This comment has been minimized.

Show comment
Hide comment
@hkaiser

hkaiser Jun 3, 2014

Member

That's merged by 91e5e11

Member

hkaiser commented Jun 3, 2014

That's merged by 91e5e11

@hkaiser hkaiser changed the title from fix more unused variable warnings to Fix more unused variable warnings Jun 3, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment