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

Tons of smaller quality improvements #1921

Merged
merged 19 commits into from Dec 27, 2015
Merged

Tons of smaller quality improvements #1921

merged 19 commits into from Dec 27, 2015

Conversation

gentryx
Copy link
Member

@gentryx gentryx commented Dec 20, 2015

This PR addresses a lot of compiler warnings WRT tidbits like extraneous semicola, missing return statements or unreachable statements.

@gentryx
Copy link
Member Author

gentryx commented Dec 22, 2015

CircleCI is reporting that the tests are failing because of a bad include order, but IIRC I didn't touch any includes.

/hpx/hpx/config.hpp:14:2: error: Boost.Config was included before the hpx config header. This might lead to subtile failures and compile errors. Please include <hpx/config.hpp> before any other boost header
#error Boost.Config was included before the hpx config header. This might lead to subtile failures and compile errors. Please include <hpx/config.hpp> before any other boost header

@hkaiser
Copy link
Member

hkaiser commented Dec 22, 2015

@gentryx don't worry about this, it's not your problem but was caused by a bad master branch at the point you branched off.

The changes LGTM.

@@ -159,7 +159,8 @@ namespace hpx { namespace applier
}

/// Schedule threads based on the given parcel
void schedule_action(parcelset::parcel p, std::size_t num_thread = -1);
void schedule_action(parcelset::parcel p, std::size_t num_thread =
(std::numeric_limits<std::size_t>::max)());
Copy link
Member

Choose a reason for hiding this comment

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

Is numeric_limits<std::size_t>::max() really the same as -1?

Copy link
Member Author

Choose a reason for hiding this comment

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

@sithhell size_t is an unsigned type.

Copy link
Member

Choose a reason for hiding this comment

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

Why not write std::size_t(-1) instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

So, what are you trying to express here? Interpretation 1: num_thread should have a default value of -1. That's impossible because size_t is an unsigned type. Interpretation 2: num_thread should have the highest possible value. Then assigning -1 isn't exactly intuitive.

IIRC -1 is an integer literal. Sign propagation rules and C++' weak typing for machine word types have the consequence that the result is the same. It's a hack my compiler complains about.

Copy link
Member

Choose a reason for hiding this comment

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

So, what are you trying to express here? Interpretation 1: num_thread should have a default value of -1. That's impossible because size_t is an unsigned type. Interpretation

It's not impossible, unsigned has modulo arithmetics so each value representation V represents any and all values in the infinite set 2^N * k + V.

IIRC -1 is an integer literal

It isn't, 1 is an integer literal, -1 is a unary expression.

It's a hack my compiler complains about.

It's not a hack, it's an idiomatic way of generating the same value representation that this patch has, and without all the cruft.

That said, an explicit cast (of any kind) would be more explicitly and should silence any complains.

Copy link
Member Author

Choose a reason for hiding this comment

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

@K-ballo WRT being idiomatic: see Thomas' first comment. Also, writing numeric_limits::max() hardly qualifies as "all the cruft".

Copy link
Member

Choose a reason for hiding this comment

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

I still think that std::size_t(-1) is conveying the intention better than numeric_limits::max(). The former is clearly defining a sentinel value, while the latter implies max() which is just confusing in this context.

@K-ballo
Copy link
Member

K-ballo commented Dec 22, 2015

std::numeric_limits lives in <limits>, every single header that uses it shall include it.

@gentryx
Copy link
Member Author

gentryx commented Dec 22, 2015

@K-ballo Fixed, thanks!

@gentryx
Copy link
Member Author

gentryx commented Dec 23, 2015

If the purpose of -1 is to serve as a magic number, then why not define a constant of that value with an appropriate name? Anyhow, I've changed all initializations to use explicit casts.

@hkaiser
Copy link
Member

hkaiser commented Dec 24, 2015

I'm fine with having a static std::size_t const any_thread = std::size_t(-1); and use this as the sentinel.

hkaiser added a commit that referenced this pull request Dec 27, 2015
Tons of smaller quality improvements
@hkaiser hkaiser merged commit b99c789 into STEllAR-GROUP:master Dec 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants