Skip to content

Conversation

@npitre
Copy link

@npitre npitre commented Jun 24, 2019

The first word is used as a pointer, meaning it is 64 bits on 64-bit
systems. To reserve it, it has to be either a pointer or a long, not an
int nor an u32_t.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Why are we using explicit longs here instead of intptr_t?

include/kernel.h Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd personally find "aligned on a word boundary" (without reference to a specific number of bytes) to be better, here and elsewhere. (Because in 20 years, kids will smile seeing such references from the dark ages of IT ;-) ).

@npitre
Copy link
Author

npitre commented Jun 25, 2019 via email

@ghost
Copy link

ghost commented Jun 25, 2019

No particular reason other that they all occupy the same space as a pointer. This could be void* just as well, and many places do that already.

To continue our discussion from your other related PRs, let's just pick a consistent standard. If long makes you happy here, great, but let's stop using intptr_t in other contexts then.

Personally I don't like the untyped data argument to the queue API that
much, especially if the first word is special. I would have created some
opaque type, say q_hook_t, to be embedded in whatever structure you want
to queue.

There's an API meeting tomorrow at 8am PST (unsure where you are), this is a good idea and maybe you join and bring this to everyone's attention. It's a good point...

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Settled my concerns about long/intptr_t etc.

Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

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

doc changes LGTM, thanks.

@npitre
Copy link
Author

npitre commented Jun 25, 2019 via email

Copy link
Contributor

@pfalcon pfalcon left a comment

Choose a reason for hiding this comment

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

To continue our discussion from your other related PRs, let's just pick a consistent standard. If long makes you happy here, great, but let's stop using intptr_t in other contexts then.

And we already had that discussion in #16559, and already chose (u)intptr_t to use, especially given that it's recommended by the C standard (at the very least, it was introduced in the standard for usages like this, from which I hope it's fair to say that the standard recommends it).

So let me kindly call for consistency (ideally in this and future PRs like this, as I get as reviewer only in subset of 64-bit support PRs).

@ghost
Copy link

ghost commented Jun 25, 2019

So let me kindly call for consistency (ideally in this and future PRs like this, as I get as reviewer only in subset of 64-bit support PRs).

That's fine with me, as long as we're consistent. But @npitre did not appear to think this was settled, as this PR demonstrates.

The first word is used as a pointer, meaning it is 64 bits on 64-bit
systems. To reserve it, it has to be either a pointer, a long, or an
intptr_t. Not an int nor an u32_t.

Signed-off-by: Nicolas Pitre <[email protected]>
@npitre
Copy link
Author

npitre commented Jun 26, 2019 via email

@andrewboie andrewboie dismissed pfalcon’s stale review June 26, 2019 00:26

Code changed to uintptr_t

@pfalcon
Copy link
Contributor

pfalcon commented Jun 26, 2019

recheck

@nashif nashif merged commit 659fa0d into zephyrproject-rtos:master Jun 26, 2019
@npitre npitre deleted the fifo branch July 19, 2019 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: API Changes to public APIs area: Bluetooth area: Documentation area: Kernel area: Tests Issues related to a particular existing or missing test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants