-
Notifications
You must be signed in to change notification settings - Fork 69
Declare ptraddr_t in (sys/)stddef.h and not in stdint.h #2495
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
base: dev
Are you sure you want to change the base?
Conversation
It has been deprecated for some time and replaced by ptraddr_t.
| #include <stdio.h> | ||
| #include <stddef.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #include <stdio.h> | |
| #include <stddef.h> | |
| #include <stddef.h> | |
| #include <stdio.h> |
But please don't just patch the subrepo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make this a separate do-no-merge commit and pull it before merging.
| */ | ||
|
|
||
| #include <sys/types.h> | ||
| #include <sys/stddef.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't sys/types.h include this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, include or define ptraddr_t itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Which would remove the need for a large chunk of this diff)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not strictly opposed (at least for internal source), but that's not the case for ptrdiff_t.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I was looking at (s)size_t, but indeed (u)intptr_t and ptrdiff_t are not. I guess the set of types in sys/types.h is the set that ye olde Unix systems defined, and then all these new-fangled types are only used by new code that knows about ISO C headers.
| typedef size_t __ptraddr_t; | ||
| #endif | ||
| typedef __ptraddr_t ptraddr_t; | ||
| typedef size_t ptraddr_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If our sys/types.h doesn't define it then OpenZFS's shouldn't really either...
| #error do not include this header, use machine/atomic.h | ||
| #endif | ||
|
|
||
| #include <sys/stddef.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this one for? The header doesn't use ptraddr_t AFAICT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{arm64,riscv}/include/atomic.h use ptraddr_t and get sys/types.h from here.
| #define _SYS_MEMRANGE_H_ | ||
|
|
||
| #include <sys/stddef.h> | ||
| #include <sys/ioccom.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alphabetise
sys/sys/signal.h
Outdated
| #define _SYS_SIGNAL_H_ | ||
|
|
||
| #include <sys/cdefs.h> | ||
| #include <sys/stddef.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should come last in this block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though this was only for the now-removed helper, so if you swap the commit order you can avoid ever needing this?
This function is only used in freebsd64_signal.c so move it there. This avoids the need for ptraddr_t to be declared in sys/signal.h.
We don't have a defintion guard for __ptraddr_t and indentical redefinitions aren't allowed in C99 to just define ptraddr_t as size_t if needed. This will be fine on any architecture OpenZFS supports.
Stick to stddef.h like ptrdiff_t.
3212807 to
66b8743
Compare
We need to pick one place for ptraddr_t and I've upstreamed it to stddef.h matching the various CHERI LLVM ports. I've removed the definition from stdint.h and removed the long unused vaddr_t while here.