Skip to content

core-local array type conversions#2277

Closed
ajkr wants to merge 4 commits intofacebook:masterfrom
ajkr:fix-win-core-local
Closed

core-local array type conversions#2277
ajkr wants to merge 4 commits intofacebook:masterfrom
ajkr:fix-win-core-local

Conversation

@ajkr
Copy link
Copy Markdown
Contributor

@ajkr ajkr commented May 11, 2017

try to clean up the type conversions and hope it passes on windows.

one interesting thing I learned is that bitshift operations are special: in x << y, the result type depends only on the type of x, unlike most arithmetic operations where the result type depends on both operands' types.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@ajkr updated the pull request - view changes

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Copy Markdown
Contributor

@siying siying left a comment

Choose a reason for hiding this comment

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

As long as it works.

// find a power of two >= num_cpus and >= 8
size_shift_ = 3;
while (1u << size_shift_ < num_cpus) {
while (1 << size_shift_ < num_cpus) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm curious why we need to cast to size_t below but not int here. Does it mean any integer is by default int?

Copy link
Copy Markdown
Contributor Author

@ajkr ajkr May 12, 2017

Choose a reason for hiding this comment

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

yes, the result of 1 << size_shift_ is an int because its left operand is an int. I cast to size_t before returning from functions so user can consistently use size_t as array indexes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

also, unbelievably, the array index on line 52 has to be a size_t to avoid the warning about bitshifting+implicit 32-to-64-bit conversion. maybe we should just disable that warning, it makes dealing with arrays + bitshifting way too annoying.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@ajkr updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@ajkr updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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.

3 participants