-
Notifications
You must be signed in to change notification settings - Fork 16
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
cleanup headers #339
cleanup headers #339
Conversation
fac346b
to
557c3dc
Compare
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.
Major and nice refactoring. I am happy that we decided to clean up kernel-libphoenix includes.
You should probably add yourself as an author to files that you majorly changed.
This refactoring doesn't solve everything (eg. types.h should be modularized) to fully conform with POSIX, but it's major improvement.
I point small things that can improve your change a little.
arch/armv7m/reboot.c
Outdated
#elif defined(__CPU_IMXRT117X) | ||
#include <phoenix/arch/imxrt1170.h> | ||
#include <phoenix/arch/armv7m/imxrt/11xx/imxrt1170.h> | ||
#else |
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 know that's not your code but maybe:
#else | |
#elif defined(__CPU_IMXRT105X) || defined(__CPU_IMXRT106X) | |
#include <phoenix/arch/armv7m/imxrt/11xx/imxrt10xx.h> | |
#else | |
#error "Unsupported TARGET" | |
#endif |
Note: maybe this ifdefined chain should be replaced by including one file, which will have those ifdefs?
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.
Thanks, for now I'll leave the ifdefs as they are because they're only needed in one file, but if ever necessary elsewhere I agree that this could be moved to separate header.
include/arch/armv7m/stdint.h
Outdated
typedef __s8 int8_t; | ||
typedef __s16 int16_t; | ||
typedef __s32 int32_t; | ||
typedef __s64 int64_t; | ||
|
||
typedef unsigned char uint8_t; | ||
typedef unsigned short uint16_t; | ||
typedef unsigned int uint32_t; | ||
typedef unsigned long long uint64_t; | ||
typedef __u8 uint8_t; | ||
typedef __u16 uint16_t; | ||
typedef __u32 uint32_t; | ||
typedef __u64 uint64_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.
Potentially it could be moved to generic include/stdint.h
Then we may want to not used it in least/fast/ptr/max types defined below or firstly typedef and then include.
side note: least type could also be moved to generic include/stdint.h in section
USE_STANDARD_TYPES_STDINT
1f47da2
to
0693b7c
Compare
include/stdint.h
Outdated
typedef uint32_t uint_least32_t; | ||
typedef uint64_t uint_least64_t; | ||
|
||
#ifdef _USE_STANDARD_TYPES_STDINT |
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.
To fully conform with idea of this define, this ifdef should be above /* 7.18.1.1 Exact-width integer types */
in fact C doesn't require to have uint(8/16/32/64)_t types defined.
It's minor as we don't want to support weird architectures but would be great to preserve idea here on wipe it out :)
JIRA: RTOS-125
JIRA: RTOS-125
JIRA: RTOS-125
0693b7c
to
9187f69
Compare
Description
Adapt headers for the common user- and kernel-space parts to be used from installed kernel headers, update types.
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist:
Special treatment