You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/11/05 06:35:59 UTC
[GitHub] [incubator-nuttx] yamt opened a new pull request #2222: sim: Use a few integer types from the compiler
yamt opened a new pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222
## Summary
The main motivaion is to allow to use -Wformat compiler warnings.
Otherwise, it complains on long vs long long differences for intmax_t.
Note: The compiler only knows the printf format and the host OS types.
It doesn't know NuttX types.
Note: On both of 64-bit macOS and Ubuntu x86_64, the host intmax_t
is "long int" while NuttX's intmax_t without this change is
"signed long long". While they are both 64-bit integers, clang -Wformat
complains on the difference.
## Impact
sim
## Testing
tested with local app on sim/macOS
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: sim: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r517858624
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
we can. i just wanted to make the change small. do you want me to go through all archs in this PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r520638346
##########
File path: arch/arm/src/lpc214x/lpc214x_serial.c
##########
@@ -89,7 +90,7 @@ static int up_attach(struct uart_dev_s *dev);
static void up_detach(struct uart_dev_s *dev);
static int up_interrupt(int irq, void *context, void *arg);
static int up_ioctl(struct file *filep, int cmd, unsigned long arg);
-static int up_receive(struct uart_dev_s *dev, uint32_t *status);
+static int up_receive(struct uart_dev_s *dev, unsigned int *status);
Review comment:
my motivation is to enable some compiler warnings about types.
unfortunately, our code base is not so consistent in that regard.
that's why i'm making this many changes in this PR and the associated PRs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: sim: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r517861736
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
Yes, I think the consistent style and equally support all arch is NuttX's unique characteristics.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r518077630
##########
File path: include/nuttx/compiler.h
##########
@@ -143,6 +143,8 @@
# define inline_function __attribute__ ((always_inline,no_instrument_function))
# define noinline_function __attribute__ ((noinline))
+# define printflike(a, b) __attribute__((__format__ (__printf__, a, b)))
Review comment:
Ok. Let's do it step by step.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r523691616
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
Sure, let's focus on clear the printf warning in this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: sim: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r517826966
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
i guess the int64 fallback in stdint.h is enough.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-728738837
All patches fix the type mismatch issue, why split it into multiple small one(e.g. sim has 4 patch). BTW, is compiler_stdint.h required?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-727930012
@yamt great work! but there are many intermediate patches, can you organize the patch series into two:
1. Fix type mismatch and nxstyle(or better to has nxstyle fix in a standalone patch)
2. inttype.h correction and printflike annotation(or better to split the annotation into another patch)
The fix for type, nxstyle and annotation is good and ready to merge, inttype.h need more review and dissuss.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-728727154
> > @yamt can you merge the patches in arch into one and printflike annotaion into another?
>
> why? i feel them separate.
There are three or four patches to modify arch/xxx/include/inttypes.h, at least we should merge these patches into one and then each arch just need one patch. And there are many patches to add printflike annonation, we should merge them into one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt edited a comment on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt edited a comment on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-728376390
i guess compilers used for arch/z80 and arch/z16 don't provide `__INTMAX_TYPE__` or `__UINTMAX_TYPE__`.
@patacongo is it right?
in that case, those types.h need to be updated to provide _intmax_t and _uintmax_t.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: sim: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r517865908
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
ok. i will take a look later.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r518001759
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
after thinking a bit more, this approach needs to use __UINT32_FMTu__ for PRIu32 etc to be consistent.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r526558013
##########
File path: arch/sim/include/inttypes.h
##########
@@ -44,7 +44,7 @@
* Pre-processor Definitions
****************************************************************************/
-#if defined(CONFIG_HOST_MACOS)
+#if defined(CONFIG_HOST_MACOS) && defined(__APPLE_CC__) || !defined(_LP64)
Review comment:
If I undersand correctly, macOS has two cases:
1. Compile the image with clang(`__INT64_TYPE__ == long long`)
2. Compile the module with gcc(`__INT64_TYPE__ == long`)
In the first case, both CONFIG_HOST_MACOS and __APPLE_CC__ are defined
In the second case, CONFIG_HOST_MACOS is defined, but __APPLE_CC__ isn't.
So the following definition should be good:
```
#if defined(__APPLE_CC__)
typedef signed long long _int64_t;
typedef unsigned long long _uint64_t;
#else
typedef signed long _int64_t;
typedef unsigned long _uint64_t;
#endif
```
If we consider Linux 32bit mode(`__INT64_TYPE__ == long long`), it become:
```
#if defined(__APPLE_CC__) || !defined(_LP64)
typedef signed long long _int64_t;
typedef unsigned long long _uint64_t;
#else
typedef signed long _int64_t;
typedef unsigned long _uint64_t;
#endif
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r517944570
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
Thanks @yamt, this patch will make we found the wrong usage of printf/scanf more easiler.
And syslog and debug.h could improve too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: sim: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r517834303
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
Review comment:
i guess some of other 64-bit archs need it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-730091948
> * the common definition doesn't work well for macOS:
>
> ```
> spacetanuki% cc -dM -E - < /dev/null|grep -E "__INT(32|64|MAX)_TYPE__"
> #define __INT32_TYPE__ int
> #define __INT64_TYPE__ long long int
> #define __INTMAX_TYPE__ long int
> spacetanuki%
> ```
Ok, thanks for explanation, it's clear now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r523297170
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
a quick googling suggested: https://docs.microsoft.com/en-us/previous-versions/visualstudio/visual-studio-2010/ms235402(v=vs.100)?redirectedfrom=MSDN#advanced-annotations
i don't know if `__xxx_TYPE__` is available there.
anyway, if we decide particular boards are always compiled with a particular compiler, we can simplify them later.
i don't feel it needs to be done in this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r526081941
##########
File path: arch/sim/include/types.h
##########
@@ -75,9 +75,16 @@ typedef unsigned int _uint32_t;
/* Note about host OS types:
* - int64_t is long long for 64-bit macOS
* - int64_t is long for Ubuntu x86-64
+ *
+ * Note for sim/macOS modules:
+ * For sim/macOS, usually x86_64-elf-gcc from homebrew is used
+ * as MODULECC. It seems to be configured as __INT64_TYPE__ == long int.
+ * The __APPLE_CC__ check below is to workaround it.
+ * (The host cc defines __APPLE_CC__, while x86_64-elf-gcc doesn't.)
+ * XXX It is a problem if you need C++ symbols in symtabs for modules.
*/
-#if defined(CONFIG_HOST_MACOS) || !defined(_LP64)
+#if defined(CONFIG_HOST_MACOS) && defined(__APPLE_CC__) || !defined(_LP64)
Review comment:
same question
##########
File path: arch/sim/include/inttypes.h
##########
@@ -44,7 +44,7 @@
* Pre-processor Definitions
****************************************************************************/
-#if defined(CONFIG_HOST_MACOS)
+#if defined(CONFIG_HOST_MACOS) && defined(__APPLE_CC__) || !defined(_LP64)
Review comment:
is it enough to only check __APPLE_CC__?
```
#if defined(__APPLE_CC__) || !defined(_LP64)
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-728757341
> All patches fix the type mismatch issue, why split it into multiple small one(e.g. sim has 4 patch).
have you read the commit messages fo those 4 patches?
> BTW, is compiler_stdint.h required?
i'm not sure if i understand the question.
currently it's used. yes.
there are ways to do this without the header. in that sense, no.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-728376390
i guess compilers used for arch/z80 and arch/z16 don't provide __INTMAX_TYPE__ or __UINTMAX_TYPE__.
@patacongo is it right?
in that case, those types.h need to be updated to provide _intmax_t and _uintmax_t.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: sim: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r517861424
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
Review comment:
Yes, I think the consistent style and equally support all arch is NuttX's unique characteristics.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r518540844
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
i'm thinking to make arch types.h like the following
so that it produces an explicit error if the compiler expectation (`__UINTxx_TYPE__`) doesn't match with ours.
```
typedef unsigned int _uint32_t;
#if defined(__UINT32_TYPE__)
typedef __UINT32_TYPE__ _uint32_t;
#endif
```
the C standard allows typedef redefinitions as far as the type is same.
(n1570.pdf 6.7/3)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r524684256
##########
File path: include/nuttx/compiler_stdint.h
##########
@@ -69,6 +69,9 @@ typedef __INTMAX_TYPE__ _intmax_t;
#if defined(__UINTMAX_TYPE__)
typedef __UINTMAX_TYPE__ _uintmax_t;
#endif
+#if defined(__SIZE_TYPE__)
+typedef __SIZE_TYPE__ _size_t;
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r518025784
##########
File path: include/nuttx/compiler.h
##########
@@ -143,6 +143,8 @@
# define inline_function __attribute__ ((always_inline,no_instrument_function))
# define noinline_function __attribute__ ((noinline))
+# define printflike(a, b) __attribute__((__format__ (__printf__, a, b)))
Review comment:
printf alone will likely need many associated fixes.
i feel it's better to leave scanf for now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-728386763
> i guess compilers used for arch/z80 and arch/z16 don't provide `__INTMAX_TYPE__` or `__UINTMAX_TYPE__`.
> @patacongo is it right?
Very unlikely. It is c89 which is before stdint.h
> in that case, those types.h need to be updated to provide _intmax_t and _uintmax_t.
You will need to add those.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-729392591
> > Yes, it is used, but all configs can build fine by removing compiler_stdint.h. So I think it is redundant and should be removed.
>
> * the majority of configs relies on it providing _uintmax_t and _intmax_t.
If we make uint64_t/int64_t same as compiler want, can typedef uintmax_t/intmax_t to uint64_t/int64_t match compiler expectation?
If the answer is false, I still prefer to typedef _uintmax_t and _intmax_t in arch's inttype.h like other type, because the consistence is also important:
```
No Short Cuts
Reducing effort at the expense of Quality, Portability, or Consistency.
```
> * it serves as an assertion to ensure the compiler types matches arch types.h.
>
The printf annotation can do the same thing, right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r520642548
##########
File path: arch/arm/src/lpc214x/lpc214x_serial.c
##########
@@ -89,7 +90,7 @@ static int up_attach(struct uart_dev_s *dev);
static void up_detach(struct uart_dev_s *dev);
static int up_interrupt(int irq, void *context, void *arg);
static int up_ioctl(struct file *filep, int cmd, unsigned long arg);
-static int up_receive(struct uart_dev_s *dev, uint32_t *status);
+static int up_receive(struct uart_dev_s *dev, unsigned int *status);
Review comment:
are you suggesting that uart_ops_s should be changed? maybe. sorry, i don't volunteer at this point.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r523297890
##########
File path: arch/arm/src/lpc214x/lpc214x_serial.c
##########
@@ -89,7 +90,7 @@ static int up_attach(struct uart_dev_s *dev);
static void up_detach(struct uart_dev_s *dev);
static int up_interrupt(int irq, void *context, void *arg);
static int up_ioctl(struct file *filep, int cmd, unsigned long arg);
-static int up_receive(struct uart_dev_s *dev, uint32_t *status);
+static int up_receive(struct uart_dev_s *dev, unsigned int *status);
Review comment:
i think it's common for compilers and lint-like tools to warn on non-portable code, even if those types happens to have the same width for the current target.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r518530196
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
unfortunately it seems that `__UINT32_FMTu__` style macros are clang-only.
i guess we just need to change the "default" types in arch types.h and inttypes.h to match what gcc expects.
for some archs, they are different right now.
eg. xtensa-esp32-elf-gcc has `#define __INT32_TYPE__ int` while we have signed long https://github.com/apache/incubator-nuttx/blob/713a21e57c27db7bddbb1e841714829d62d5ef7d/arch/xtensa/include/types.h#L72
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r518574778
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
because some compilers might not define `__UINT32_TYPE__`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: sim: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r517844496
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
Review comment:
It isn't specific to 64bit arch, I think. Instead, it couple with toolchain(gcc or clang specific). and tt's easy to apply and verify on other arch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-729251876
> > > All patches fix the type mismatch issue, why split it into multiple small one(e.g. sim has 4 patch).
> >
> >
> > have you read the commit messages fo those 4 patches?
> > > BTW, is compiler_stdint.h required?
> >
> >
> > i'm not sure if i understand the question.
> > currently it's used. yes.
>
> Yes, it is used, but all configs can build fine by removing compiler_stdint.h. So I think it is redundant and should be removed.
* the majority of configs relies on it providing _uintmax_t and _intmax_t.
* it serves as an assertion to ensure the compiler types matches arch types.h.
>
> > there are ways to do this without the header. in that sense, no.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-729650746
> > > > > Yes, it is used, but all configs can build fine by removing compiler_stdint.h. So I think it is redundant and should be removed.
> > > >
> > > >
> > > >
> > > > * the majority of configs relies on it providing _uintmax_t and _intmax_t.
> > >
> > >
> > > If we make uint64_t/int64_t same as compiler want, can typedef uintmax_t/intmax_t to uint64_t/int64_t match compiler expectation?
> > > If the answer is false, I still prefer to typedef _uintmax_t and _intmax_t in arch's inttype.h like other type, because the consistence is also important:
> > > ```
> > > No Short Cuts
> > > Reducing effort at the expense of Quality, Portability, or Consistency.
> > > ```
> >
> >
> > ok, it makes sense.
> > > > * it serves as an assertion to ensure the compiler types matches arch types.h.
> > >
> > >
> > > The printf annotation can do the same thing, right?
> >
> >
> > i don't know how compiler printf format checks works wrt default type promotion.
> > even if it works, i don't think it's a good idea to rely on that level of details.
>
> But, since all libc funcitons are implemented by NuttX self, how to map these basic types should be determined by NuttX too. We have to match the compiler expectation here, just because we want to utilize the printf/scanf format specifier chceck provided by compiler. So it's reasonable to limit the mismatch in printf/scanf format checker.
libc is just a part of C runtime. the compiler is also a fundamental part of it.
it's better to ensure type expectations matched unless you are seeking problems.
having said that, i dropped the compiler_stdint.h stuff from this PR because it turned out to be more controversial than i expected.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-728733157
> > > @yamt can you merge the patches in arch into one and printflike annotaion into another?
> >
> >
> > why? i feel them separate.
>
> There are three or four patches to modify arch/xxx/include/inttypes.h, at least we should merge these patches into one and then each arch just need one patch. And there are many patches to add printflike annonation, we should merge them into one.
what's wrong with having multiple patches for an arch? i think they are logically separate.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-728386763
> i guess compilers used for arch/z80 and arch/z16 don't provide `__INTMAX_TYPE__` or `__UINTMAX_TYPE__`.
> @patacongo is it right?
Very unlikely. It is c89.
> in that case, those types.h need to be updated to provide _intmax_t and _uintmax_t.
You will need to add those.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r526558013
##########
File path: arch/sim/include/inttypes.h
##########
@@ -44,7 +44,7 @@
* Pre-processor Definitions
****************************************************************************/
-#if defined(CONFIG_HOST_MACOS)
+#if defined(CONFIG_HOST_MACOS) && defined(__APPLE_CC__) || !defined(_LP64)
Review comment:
If I undersand correctly, macOS has two cases:
1. Compile the image with clang(__INT64_TYPE__ == long long)
2. Compile the module with gcc(__INT64_TYPE__ == long)
In the first case, both CONFIG_HOST_MACOS and __APPLE_CC__ are defined
In the second case, CONFIG_HOST_MACOS is defined, but __APPLE_CC__ isn't.
So the following definition should be good:
```
#if defined(__APPLE_CC__)
typedef signed long long _int64_t;
typedef unsigned long long _uint64_t;
#else
typedef signed long _int64_t;
typedef unsigned long _uint64_t;
#endif
```
If we consider Linux 32bit mode(__INT64_TYPE__ == long long), it become:
```
#if defined(__APPLE_CC__) || !defined(_LP64)
typedef signed long long _int64_t;
typedef unsigned long long _uint64_t;
#else
typedef signed long _int64_t;
typedef unsigned long _uint64_t;
#endif
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-730093406
These two patch should merge into one:
https://github.com/apache/incubator-nuttx/pull/2222/commits/9bc2c990724d6240699cfdacab67edaf35ba790a
https://github.com/apache/incubator-nuttx/pull/2222/commits/45a1d681a9480b87fa16e59305ef08433330e03a
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r524856580
##########
File path: arch/sim/include/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
So, I think that it's enough to correct typedef to ensure _xxx same as what compiler want. compiler_stdint.h is redundant and make the confusion(why define the same type twice) because the compiler can generate the mismatch warning even without including compiler_stdint.h
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r517932311
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
i updated all types.h except arch/z80/include/chip/types.h.
the arch/z80/include/chip directory seems like a stale copy of arch/z80/include/z8.
@patacongo should arch/z80/include/chip/types.h be identical to arch/z80/include/z8/types.h?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-728658425
@yamt can you merge the patches in arch into one and printflike annotaion into another?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r524683998
##########
File path: include/stdio.h
##########
@@ -190,15 +190,21 @@ int snprintf(FAR char *buf, size_t size,
int sscanf(FAR const char *buf, FAR const IPTR char *fmt, ...);
int scanf(FAR const IPTR char *fmt, ...);
-int vasprintf(FAR char **ptr, FAR const IPTR char *fmt, va_list ap);
+int vasprintf(FAR char **ptr, FAR const IPTR char *fmt, va_list ap)
+ printflike(2, 0);
int vfprintf(FAR FILE *stream, FAR const IPTR char *fmt,
- va_list ap);
-int vfscanf(FAR FILE *stream, FAR const IPTR char *fmt, va_list ap);
-int vprintf(FAR const IPTR char *fmt, va_list ap);
+ va_list ap)
+ printflike(2, 0);
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r526458158
##########
File path: arch/sim/include/inttypes.h
##########
@@ -44,7 +44,7 @@
* Pre-processor Definitions
****************************************************************************/
-#if defined(CONFIG_HOST_MACOS)
+#if defined(CONFIG_HOST_MACOS) && defined(__APPLE_CC__) || !defined(_LP64)
Review comment:
no. it's for MODULECC. (see the commit message)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-729507112
> > > > Yes, it is used, but all configs can build fine by removing compiler_stdint.h. So I think it is redundant and should be removed.
> > >
> > >
> > >
> > > * the majority of configs relies on it providing _uintmax_t and _intmax_t.
> >
> >
> > If we make uint64_t/int64_t same as compiler want, can typedef uintmax_t/intmax_t to uint64_t/int64_t match compiler expectation?
> > If the answer is false, I still prefer to typedef _uintmax_t and _intmax_t in arch's inttype.h like other type, because the consistence is also important:
> > ```
> > No Short Cuts
> > Reducing effort at the expense of Quality, Portability, or Consistency.
> > ```
>
> ok, it makes sense.
>
> > > * it serves as an assertion to ensure the compiler types matches arch types.h.
> >
> >
> > The printf annotation can do the same thing, right?
>
> i don't know how compiler printf format checks works wrt default type promotion.
> even if it works, i don't think it's a good idea to rely on that level of details.
But, since all libc funcitons are implemented by NuttX self, how to map these basic types should be determined by NuttX too. We have to match the compiler expectation here, just because we want to utilize the printf/scanf format specifier chceck provided by compiler. So it's reasonable to limit the mismatch in printf/scanf format checker.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-728727154
> > @yamt can you merge the patches in arch into one and printflike annotaion into another?
>
> why? i feel them separate.
There are three or four patches to modify arch/xxx/include/inttypes.h, at least we should only merge these patches into one. And there are many patches to add printflike annonation, we should merge into one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: sim: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r517826060
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
Review comment:
i don't know. as i haven't checked other archs yet.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-729679114
@yamt after reviewing the final patchset, I found that the common typedef for intmax_t/uintmax_t in include/stdint.h:
```
#ifdef __INT64_DEFINED
typedef _int64_t intmax_t;
typedef _uint64_t uintmax_t;
#else
typedef _int32_t intmax_t;
typedef _uint32_t uintmax_t;
#endif
```
should work fine after we correct _int64_t/_uint64_t/_int32_t/_uint32_t. Why we move intmax_t/uintmax_t to arch/inttype.h?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-729679114
@yamt after reviewing the final patchset, I found that the common typedef for intmax_t/uintmax_t should work fine after we correct _int64_t/_uint64_t/_int32_t/_uint32_t:
```
#ifdef __INT64_DEFINED
typedef _int64_t intmax_t;
typedef _uint64_t uintmax_t;
#else
typedef _int32_t intmax_t;
typedef _uint32_t uintmax_t;
#endif
```
Why we move intmax_t/uintmax_t to arch/inttype.h?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-729679114
@yamt after reviewing the final patchset, I found that the common typedef for intmax_t/uintmax_t:
```
#ifdef __INT64_DEFINED
typedef _int64_t intmax_t;
typedef _uint64_t uintmax_t;
#else
typedef _int32_t intmax_t;
typedef _uint32_t uintmax_t;
#endif
```
should work fine after we correct _int64_t/_uint64_t/_int32_t/_uint32_t. Why we move intmax_t/uintmax_t to arch/inttype.h?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r518017297
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
Yes, you are right.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r520634422
##########
File path: arch/arm/src/lpc214x/lpc214x_serial.c
##########
@@ -89,7 +90,7 @@ static int up_attach(struct uart_dev_s *dev);
static void up_detach(struct uart_dev_s *dev);
static int up_interrupt(int irq, void *context, void *arg);
static int up_ioctl(struct file *filep, int cmd, unsigned long arg);
-static int up_receive(struct uart_dev_s *dev, uint32_t *status);
+static int up_receive(struct uart_dev_s *dev, unsigned int *status);
Review comment:
"Right now uint32_t is used everywhere" is a false assumption.
please see the interface definition here:
https://github.com/apache/incubator-nuttx/blob/c79bda6e4f305c153a117e7ad454ec4e69610d69/include/nuttx/serial/serial.h#L206-L211
also, there is a precedent:
https://github.com/apache/incubator-nuttx/commit/e2aad9c05a085abb9d58b1faa70a631be5e8d89f
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-728727154
> > @yamt can you merge the patches in arch into one and printflike annotaion into another?
>
> why? i feel them separate.
There are three or four patches to modify arch/xxx/include/inttypes.h, at least we should merge these patches into one and then each arch just need one patch. And there are many patches to add printflike annonation, we should merge into one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r520647569
##########
File path: arch/arm/src/lpc214x/lpc214x_serial.c
##########
@@ -89,7 +90,7 @@ static int up_attach(struct uart_dev_s *dev);
static void up_detach(struct uart_dev_s *dev);
static int up_interrupt(int irq, void *context, void *arg);
static int up_ioctl(struct file *filep, int cmd, unsigned long arg);
-static int up_receive(struct uart_dev_s *dev, uint32_t *status);
+static int up_receive(struct uart_dev_s *dev, unsigned int *status);
Review comment:
I was merely asking, trying to understand the context behind this.
I understand now that you are trying to match the declaration of the callback. That sounds OK.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r524192384
##########
File path: arch/sim/include/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
because
* i'm not comfortable to assume that `__xxx_TYPE__` is always available.
* predefined macros are not obvious when reading the code.
* this way we can easily notice possible mistakes (like using a compiler which is configured in an incompatible way)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-728717616
> @yamt can you merge the patches in arch into one and printflike annotaion into another?
why? i feel them separate.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r517947462
##########
File path: include/nuttx/compiler.h
##########
@@ -143,6 +143,8 @@
# define inline_function __attribute__ ((always_inline,no_instrument_function))
# define noinline_function __attribute__ ((noinline))
+# define printflike(a, b) __attribute__((__format__ (__printf__, a, b)))
Review comment:
need scanf format
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-728726772
i dropped printflike stuff from this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: sim: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r517844496
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
Review comment:
It isn't specific to 64bit arch, I think. Instead, it couple with toolchain(gcc or clang specific). It's easy to apply to other arch.
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
but why not to make _intmax_t similar as other type? anyway, you need define _intmax_t in arch.h now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-727883759
finally the ci is green. yay.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r526558013
##########
File path: arch/sim/include/inttypes.h
##########
@@ -44,7 +44,7 @@
* Pre-processor Definitions
****************************************************************************/
-#if defined(CONFIG_HOST_MACOS)
+#if defined(CONFIG_HOST_MACOS) && defined(__APPLE_CC__) || !defined(_LP64)
Review comment:
If I undersand correctly, macOS has two cases:
1. Compile the image with clang(`__INT64_TYPE__ == long long`)
2. Compile the module with gcc(`__INT64_TYPE__ == long`)
In the first case, both CONFIG_HOST_MACOS and `__APPLE_CC__` are defined
In the second case, CONFIG_HOST_MACOS is defined, but `__APPLE_CC__` isn't.
So the following definition should be good:
```
#if defined(__APPLE_CC__)
typedef signed long long _int64_t;
typedef unsigned long long _uint64_t;
#else
typedef signed long _int64_t;
typedef unsigned long _uint64_t;
#endif
```
If we consider Linux 32bit mode(`__INT64_TYPE__ == long long`), it become:
```
#if defined(__APPLE_CC__) || !defined(_LP64)
typedef signed long long _int64_t;
typedef unsigned long long _uint64_t;
#else
typedef signed long _int64_t;
typedef unsigned long _uint64_t;
#endif
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: sim: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r517823474
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
how about we also typedef _intmax_t in __INTMAX_TYPE__ undefined case?
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
Review comment:
should we apply the similar change to other arch? so we can decorate all printf/scanf like function with "format(...)" to catch the wrong type.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r524198512
##########
File path: arch/sim/include/inttypes.h
##########
@@ -44,7 +44,7 @@
* Pre-processor Definitions
****************************************************************************/
-#if defined(__APPLE__)
+#if defined(CONFIG_HOST_MACOS)
Review comment:
because of https://github.com/apache/incubator-nuttx/commit/b82756539623c2561c0ab612b2089e111399cbd0
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r518597053
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
In that case, I suppose print_format also doesn't support too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r520639363
##########
File path: arch/arm/src/lpc214x/lpc214x_serial.c
##########
@@ -89,7 +90,7 @@ static int up_attach(struct uart_dev_s *dev);
static void up_detach(struct uart_dev_s *dev);
static int up_interrupt(int irq, void *context, void *arg);
static int up_ioctl(struct file *filep, int cmd, unsigned long arg);
-static int up_receive(struct uart_dev_s *dev, uint32_t *status);
+static int up_receive(struct uart_dev_s *dev, unsigned int *status);
Review comment:
Well, what I meant is that in some architectures (for example, nRF52) uint32_t is the type used to handle register values and not unsigned int. Similarly, for 16 bit values you can see uint16_t used extensively, instead of unsigned short.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: sim: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r517849449
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
> but why not to make _intmax_t similar as other type?
just because i didn't need to change it.
> anyway, you need define _intmax_t in arch.h now.
can you explain?
in my patch, _intmax_t is used only when __INTMAX_DEFINED is defined.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r526558013
##########
File path: arch/sim/include/inttypes.h
##########
@@ -44,7 +44,7 @@
* Pre-processor Definitions
****************************************************************************/
-#if defined(CONFIG_HOST_MACOS)
+#if defined(CONFIG_HOST_MACOS) && defined(__APPLE_CC__) || !defined(_LP64)
Review comment:
If I undersand correctly, macOS has two cases:
1. Compile the image with clang(`__INT64_TYPE__ == long long`)
2. Compile the module with gcc(`__INT64_TYPE__ == long`)
In the first case, both CONFIG_HOST_MACOS and `__APPLE_CC__` are defined
In the second case, CONFIG_HOST_MACOS is defined, but `__APPLE_CC__` isn't.
So the following definition should be good:
```
#if defined(__APPLE_CC__)
typedef signed long long _int64_t;
typedef unsigned long long _uint64_t;
#else
typedef signed long _int64_t;
typedef unsigned long _uint64_t;
#endif
```
If we consider Linux 32bit mode(`__INT64_TYPE__ == long long`), it become:
```
#if defined(__APPLE_CC__) || !defined(_LP64)
typedef signed long long _int64_t;
typedef unsigned long long _uint64_t;
#else
typedef signed long _int64_t;
typedef unsigned long _uint64_t;
#endif
```
Actually, the real difference is which compiler is used(gcc or clang), clang could be detected by `__APPLE_CC__`. the typedef for gcc is same for macOS and Linux(but 32bit/64bit mode has the different typedef).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-728764827
> > All patches fix the type mismatch issue, why split it into multiple small one(e.g. sim has 4 patch).
>
> have you read the commit messages fo those 4 patches?
>
> > BTW, is compiler_stdint.h required?
>
> i'm not sure if i understand the question.
> currently it's used. yes.
Yes, it is used, but all configs can build fine by removing compiler_stdint.h. So I think it is redundant and should be removed.
> there are ways to do this without the header. in that sense, no.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-729994654
> @yamt after reviewing the final patchset, I found that the common typedef for intmax_t/uintmax_t in include/stdint.h:
>
> ```
> #ifdef __INT64_DEFINED
> typedef _int64_t intmax_t;
> typedef _uint64_t uintmax_t;
> #else
> typedef _int32_t intmax_t;
> typedef _uint32_t uintmax_t;
> #endif
> ```
>
> should work fine after we correct _int64_t/_uint64_t/_int32_t/_uint32_t. Why we move intmax_t/uintmax_t to arch/inttype.h?
* you suggested it's better to have them in arch types.h for consistency with other types and i agreed.
* the common definition doesn't work well for macOS:
```
spacetanuki% cc -dM -E - < /dev/null|grep -E "__INT(32|64|MAX)_TYPE__"
#define __INT32_TYPE__ int
#define __INT64_TYPE__ long long int
#define __INTMAX_TYPE__ long int
spacetanuki%
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r517944570
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
Thanks @yamt, this patch will make we found the wrong usage of printf/scanf more easiler.
And scanf, syslog and debug.h could improve too.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r524082178
##########
File path: include/stdio.h
##########
@@ -190,15 +190,21 @@ int snprintf(FAR char *buf, size_t size,
int sscanf(FAR const char *buf, FAR const IPTR char *fmt, ...);
int scanf(FAR const IPTR char *fmt, ...);
-int vasprintf(FAR char **ptr, FAR const IPTR char *fmt, va_list ap);
+int vasprintf(FAR char **ptr, FAR const IPTR char *fmt, va_list ap)
+ printflike(2, 0);
int vfprintf(FAR FILE *stream, FAR const IPTR char *fmt,
- va_list ap);
-int vfscanf(FAR FILE *stream, FAR const IPTR char *fmt, va_list ap);
-int vprintf(FAR const IPTR char *fmt, va_list ap);
+ va_list ap)
+ printflike(2, 0);
Review comment:
merge to previous line
##########
File path: arch/xtensa/include/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: include/nuttx/compiler_stdint.h
##########
@@ -69,6 +69,9 @@ typedef __INTMAX_TYPE__ _intmax_t;
#if defined(__UINTMAX_TYPE__)
typedef __UINTMAX_TYPE__ _uintmax_t;
#endif
+#if defined(__SIZE_TYPE__)
+typedef __SIZE_TYPE__ _size_t;
Review comment:
merge into one patch?
##########
File path: arch/x86/include/i486/types.h
##########
@@ -45,6 +45,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: arch/arm/include/types.h
##########
@@ -30,6 +30,7 @@
****************************************************************************/
#include <nuttx/config.h>
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: arch/risc-v/include/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: arch/hc/include/hcs12/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: arch/xtensa/src/esp32/esp32_wifi_adapter.c
##########
@@ -389,7 +390,7 @@ wifi_osi_funcs_t g_wifi_osi_funcs =
._nvs_erase_key = esp_nvs_erase_key,
._get_random = esp_get_random,
._get_time = esp_get_time,
- ._random = esp_random,
+ ._random = esp_random_ulong,
Review comment:
should w change _ranom prototype to uint32_t (void)?
##########
File path: arch/avr/include/avr/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: arch/x86_64/include/intel64/types.h
##########
@@ -30,6 +30,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: arch/avr/include/avr32/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: arch/renesas/include/m16c/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: include/stdio.h
##########
@@ -190,15 +190,21 @@ int snprintf(FAR char *buf, size_t size,
int sscanf(FAR const char *buf, FAR const IPTR char *fmt, ...);
int scanf(FAR const IPTR char *fmt, ...);
-int vasprintf(FAR char **ptr, FAR const IPTR char *fmt, va_list ap);
+int vasprintf(FAR char **ptr, FAR const IPTR char *fmt, va_list ap)
+ printflike(2, 0);
int vfprintf(FAR FILE *stream, FAR const IPTR char *fmt,
- va_list ap);
-int vfscanf(FAR FILE *stream, FAR const IPTR char *fmt, va_list ap);
-int vprintf(FAR const IPTR char *fmt, va_list ap);
+ va_list ap)
+ printflike(2, 0);
+int vfscanf(FAR FILE *stream, FAR const IPTR char *fmt, va_list ap)
+ printflike(2, 0);
+int vprintf(FAR const IPTR char *fmt, va_list ap)
+ printflike(1, 0);
int vscanf(FAR const IPTR char *fmt, va_list ap);
int vsnprintf(FAR char *buf, size_t size, FAR const IPTR char *fmt,
- va_list ap);
-int vsprintf(FAR char *buf, FAR const IPTR char *fmt, va_list ap);
+ va_list ap)
+ printflike(3, 0);
Review comment:
merge to previous line
##########
File path: arch/misoc/include/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: arch/sim/include/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
if we include compiler_stdint.h, why not remove all from line 68?
##########
File path: arch/renesas/include/rx65n/types.h
##########
@@ -29,6 +29,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: arch/or1k/include/types.h
##########
@@ -45,6 +45,7 @@
****************************************************************************/
#include <nuttx/config.h>
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: arch/z80/include/z80/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: arch/z80/include/z8/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: arch/renesas/include/sh1/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: arch/sim/include/inttypes.h
##########
@@ -44,7 +44,7 @@
* Pre-processor Definitions
****************************************************************************/
-#if defined(__APPLE__)
+#if defined(CONFIG_HOST_MACOS)
Review comment:
why not use __APPLE__?
##########
File path: arch/z16/include/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: arch/mips/include/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
why include compiler_stdint.h
##########
File path: arch/z80/include/ez80/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: arch/hc/include/hc12/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
##########
File path: arch/z80/include/z180/types.h
##########
@@ -44,6 +44,8 @@
* Included Files
****************************************************************************/
+#include <nuttx/compiler_stdint.h>
Review comment:
same question as sim
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: sim: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r517856294
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
I mean that intmax_t just need appear in stdint.h before your patch, it's reasonable to different from other basic type. But, now we have to typedef _intmax_t in arch.h just like other basic type, why not use the same approach to always typedef _intmax_t?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r520650306
##########
File path: arch/arm/src/lpc214x/lpc214x_serial.c
##########
@@ -89,7 +90,7 @@ static int up_attach(struct uart_dev_s *dev);
static void up_detach(struct uart_dev_s *dev);
static int up_interrupt(int irq, void *context, void *arg);
static int up_ioctl(struct file *filep, int cmd, unsigned long arg);
-static int up_receive(struct uart_dev_s *dev, uint32_t *status);
+static int up_receive(struct uart_dev_s *dev, unsigned int *status);
Review comment:
And actually I was also curious because I'm used to get a warning from QtCreator's clangbackend whenever I use putreg32, even if I pass uint32_t, it complains that I'm casting an integer to a pointer, which it understands it is not the same size. I always understood this as clangbackend not getting the correct flags, but at the same time I wondered if we should be using uintptr_t in some places instead of uint32_t*. Anyway, just wondering about it, not suggesting any changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-730188637
> These two patch should merge into one:
> [9bc2c99](https://github.com/apache/incubator-nuttx/commit/9bc2c990724d6240699cfdacab67edaf35ba790a)
> [45a1d68](https://github.com/apache/incubator-nuttx/commit/45a1d681a9480b87fa16e59305ef08433330e03a)
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r526648730
##########
File path: arch/sim/include/inttypes.h
##########
@@ -44,7 +44,7 @@
* Pre-processor Definitions
****************************************************************************/
-#if defined(CONFIG_HOST_MACOS)
+#if defined(CONFIG_HOST_MACOS) && defined(__APPLE_CC__) || !defined(_LP64)
Review comment:
done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: sim: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r517847986
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
Review comment:
i suppose that the specific issue i wanted to fix (ie. long long vs long mismatch) is only for 64-bit archs.
i agree it can be considered more general though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r520624418
##########
File path: arch/arm/src/lpc214x/lpc214x_serial.c
##########
@@ -89,7 +90,7 @@ static int up_attach(struct uart_dev_s *dev);
static void up_detach(struct uart_dev_s *dev);
static int up_interrupt(int irq, void *context, void *arg);
static int up_ioctl(struct file *filep, int cmd, unsigned long arg);
-static int up_receive(struct uart_dev_s *dev, uint32_t *status);
+static int up_receive(struct uart_dev_s *dev, unsigned int *status);
Review comment:
I might be a bit out of context (I think you already worked on similar changes in other PRs) but what is the motivation behind this change? I understand that since you know which platform you are in you already now that unsigned int is 32 bit, so no need to explicitly say so. However, aren't we then introducing inconsistent practices in the code? Right now uint32_t is used everywhere and it would be wrong to see some functions use one type and others another type (regardless of actually being the same size).
Also, why is uint32_t a problem? Isn't this definition standard? (defined in stdint.h after all)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-729398667
> > > Yes, it is used, but all configs can build fine by removing compiler_stdint.h. So I think it is redundant and should be removed.
> >
> >
> >
> > * the majority of configs relies on it providing _uintmax_t and _intmax_t.
>
> If we make uint64_t/int64_t same as compiler want, can typedef uintmax_t/intmax_t to uint64_t/int64_t match compiler expectation?
>
> If the answer is false, I still prefer to typedef _uintmax_t and _intmax_t in arch's inttype.h like other type, because the consistence is also important:
>
> ```
> No Short Cuts
> Reducing effort at the expense of Quality, Portability, or Consistency.
> ```
ok, it makes sense.
>
> > * it serves as an assertion to ensure the compiler types matches arch types.h.
>
> The printf annotation can do the same thing, right?
i don't know how compiler printf format checks works wrt default type promotion.
even if it works, i don't think it's a good idea to rely on that level of details.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r524194680
##########
File path: arch/xtensa/src/esp32/esp32_wifi_adapter.c
##########
@@ -389,7 +390,7 @@ wifi_osi_funcs_t g_wifi_osi_funcs =
._nvs_erase_key = esp_nvs_erase_key,
._get_random = esp_get_random,
._get_time = esp_get_time,
- ._random = esp_random,
+ ._random = esp_random_ulong,
Review comment:
the prototype is from ESP-IDF, which is not under our control.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-728386763
> i guess compilers used for arch/z80 and arch/z16 don't provide `__INTMAX_TYPE__` or `__UINTMAX_TYPE__`.
> @patacongo is it right?
Very unlikely
> in that case, those types.h need to be updated to provide _intmax_t and _uintmax_t.
You will need to add those.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-729507112
> > > > Yes, it is used, but all configs can build fine by removing compiler_stdint.h. So I think it is redundant and should be removed.
> > >
> > >
> > >
> > > * the majority of configs relies on it providing _uintmax_t and _intmax_t.
> >
> >
> > If we make uint64_t/int64_t same as compiler want, can typedef uintmax_t/intmax_t to uint64_t/int64_t match compiler expectation?
> > If the answer is false, I still prefer to typedef _uintmax_t and _intmax_t in arch's inttype.h like other type, because the consistence is also important:
> > ```
> > No Short Cuts
> > Reducing effort at the expense of Quality, Portability, or Consistency.
> > ```
>
> ok, it makes sense.
>
> > > * it serves as an assertion to ensure the compiler types matches arch types.h.
> >
> >
> > The printf annotation can do the same thing, right?
>
> i don't know how compiler printf format checks works wrt default type promotion.
> even if it works, i don't think it's a good idea to rely on that level of details.
But, since all libc funcitons are implemented by NuttX self, how to map these basic types should be determined by NuttX too. We have to match the compiler expectation here, just because we want to utilize the printf/scanf format specifier chceck provided by compiler.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#issuecomment-728460049
> > i guess compilers used for arch/z80 and arch/z16 don't provide `__INTMAX_TYPE__` or `__UINTMAX_TYPE__`.
> > @patacongo is it right?
>
> Very unlikely. It is c89 which is before stdint.h
>
> > in that case, those types.h need to be updated to provide _intmax_t and _uintmax_t.
>
> You will need to add those.
thank you. i added them.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2222: Use a few integer types from the compiler
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2222:
URL: https://github.com/apache/incubator-nuttx/pull/2222#discussion_r518573401
##########
File path: arch/sim/include/types.h
##########
@@ -69,13 +69,41 @@ typedef unsigned char _uint8_t;
typedef signed short _int16_t;
typedef unsigned short _uint16_t;
+#if defined(__INT32_TYPE__)
+typedef __INT32_TYPE__ _int32_t;
+#else
typedef signed int _int32_t;
+#endif
+
+#if defined(__UINT32_TYPE__)
+typedef __UINT32_TYPE__ _uint32_t;
+#else
typedef unsigned int _uint32_t;
+#endif
+#if defined(__INT64_TYPE__)
+typedef __INT64_TYPE__ _int64_t;
+#else
typedef signed long long _int64_t;
+#endif
+
+#if defined(__UINT64_TYPE__)
+typedef __UINT64_TYPE__ _uint64_t;
+#else
typedef unsigned long long _uint64_t;
+#endif
#define __INT64_DEFINED
+#if defined(__INTMAX_TYPE__)
+typedef __INTMAX_TYPE__ _intmax_t;
+#define __INTMAX_DEFINED
Review comment:
why not directly typedef __UINT32_TYPE__ _uint32_t?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org