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 2021/02/17 22:17:22 UTC
[GitHub] [incubator-nuttx] gustavonihei opened a new pull request #2863: stdint.h: Use conversion macros for the definition of MIN and MAX constants
gustavonihei opened a new pull request #2863:
URL: https://github.com/apache/incubator-nuttx/pull/2863
## Summary
Currently `stdint.h` defines the MIN and MAX macros with a suffix that may not be adequate for some CPU architectures.
E.g.: `UINT32_MAX` is `4294967295u`, but for RISC-V it should be defined as `4294967295ul`, so it also becomes consistent with the `PRIu32` definition for RISC-V (`lu`).
The motivation of this PR originated from the compilers warnings when using INT32_MAX constant with printf.
## Impact
EZ80 architecture defines `__INT24_DEFINED`, so it becomes necessary to define `INT24_C` and `UINT24_C`, which are not standardized but seems to be commonly defined in some compilers (e.g. LLVM).
For the other architectures, there should be no impact.
## Testing
Successful build and no more printf format warnings when using MIN and MAX macros.
----------------------------------------------------------------
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] davids5 commented on pull request #2863: stdint.h: Use conversion macros for the definition of MIN and MAX constants
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2863:
URL: https://github.com/apache/incubator-nuttx/pull/2863#issuecomment-781556860
What is wrong with using simple to grock numbers that just make sense.
```
# define INT8_MAX INT8_C(127)
# define INT8_MIN (-INT8_C(127)-1)
# define UINT8_MAX UINT8_C(255)
# define __INT_LEAST8_MIN INT8_MIN
# define __INT_LEAST8_MAX INT8_MAX
# define __UINT_LEAST8_MAX UINT8_MAX
```
----------------------------------------------------------------
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 #2863: stdint.h: Use conversion macros for the definition of MIN and MAX constants
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #2863:
URL: https://github.com/apache/incubator-nuttx/pull/2863#issuecomment-781772479
@davids5 let's merge this patch first, we can provide a new PR to address the issue you mentioned since it doesn't really relate to 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] davids5 edited a comment on pull request #2863: stdint.h: Use conversion macros for the definition of MIN and MAX constants
Posted by GitBox <gi...@apache.org>.
davids5 edited a comment on pull request #2863:
URL: https://github.com/apache/incubator-nuttx/pull/2863#issuecomment-781556860
What is wrong with using simple to grock numbers that just make sense?
```
# define INT8_MAX INT8_C(127)
# define INT8_MIN (-INT8_C(127)-1)
# define UINT8_MAX UINT8_C(255)
# define __INT_LEAST8_MIN INT8_MIN
# define __INT_LEAST8_MAX INT8_MAX
# define __UINT_LEAST8_MAX UINT8_MAX
```
----------------------------------------------------------------
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 #2863: stdint.h: Use conversion macros for the definition of MIN and MAX constants
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2863:
URL: https://github.com/apache/incubator-nuttx/pull/2863#issuecomment-781772479
@davids5 let's merge this patch first, we can provide a new PR to address the issue you mentioned.
----------------------------------------------------------------
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] gustavonihei commented on pull request #2863: stdint.h: Use conversion macros for the definition of MIN and MAX constants
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #2863:
URL: https://github.com/apache/incubator-nuttx/pull/2863#issuecomment-781569398
> What is wrong with using simple to grock numbers that just make sense?
>
> ```
> # define INT8_MAX INT8_C(127)
> # define INT8_MIN (-INT8_C(127)-1)
> # define UINT8_MAX UINT8_C(255)
> # define __INT_LEAST8_MIN INT8_MIN
> # define __INT_LEAST8_MAX INT8_MAX
> # define __UINT_LEAST8_MAX UINT8_MAX
> ```
I agree that the grouping of these macros should follow a given rule, which as we can see from the comments that there is a rationale behind the sorting.
For me it's okay the way it currently is.
----------------------------------------------------------------
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] gustavonihei commented on a change in pull request #2863: stdint.h: Use conversion macros for the definition of MIN and MAX constants
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #2863:
URL: https://github.com/apache/incubator-nuttx/pull/2863#discussion_r578599141
##########
File path: include/stdint.h
##########
@@ -41,28 +41,28 @@
/* Limits of exact-width integer types */
-#define INT8_MIN (-INT8_MAX - 1)
-#define INT8_MAX 127
-#define UINT8_MAX 255
+#define INT8_MIN (-INT8_MAX - INT8_C(1))
Review comment:
Sorry, I couldn't follow. Which promotion do you expect to happen in this scenario?
I don't think it is really needed, since INTx_MAX should be already suffixed (if needed by the arch) and thus it won't be promoted for after the subtraction.
Regarding `-128`, I feel that making it relative to `INT8_MAX` definition contributes with code comprehension.
And also it is the same approach used by LLVM:
https://clang.llvm.org/doxygen/stdint_8h_source.html
----------------------------------------------------------------
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 #2863: stdint.h: Use conversion macros for the definition of MIN and MAX constants
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2863:
URL: https://github.com/apache/incubator-nuttx/pull/2863#discussion_r578075667
##########
File path: include/stdint.h
##########
@@ -41,28 +41,28 @@
/* Limits of exact-width integer types */
-#define INT8_MIN (-INT8_MAX - 1)
-#define INT8_MAX 127
-#define UINT8_MAX 255
+#define INT8_MIN (-INT8_MAX - INT8_C(1))
Review comment:
should we use INT8_C(-INT8_MAX - 1) to avoid the default promotion?
##########
File path: include/stdint.h
##########
@@ -41,28 +41,28 @@
/* Limits of exact-width integer types */
-#define INT8_MIN (-INT8_MAX - 1)
-#define INT8_MAX 127
-#define UINT8_MAX 255
+#define INT8_MIN (-INT8_MAX - INT8_C(1))
+#define INT8_MAX INT8_C(127)
+#define UINT8_MAX UINT8_C(255)
-#define INT16_MIN (-INT16_MAX - 1)
-#define INT16_MAX 32767
-#define UINT16_MAX 65535u
+#define INT16_MIN (-INT16_MAX - INT16_C(1))
Review comment:
ditto
##########
File path: include/stdint.h
##########
@@ -41,28 +41,28 @@
/* Limits of exact-width integer types */
-#define INT8_MIN (-INT8_MAX - 1)
-#define INT8_MAX 127
-#define UINT8_MAX 255
+#define INT8_MIN (-INT8_MAX - INT8_C(1))
+#define INT8_MAX INT8_C(127)
+#define UINT8_MAX UINT8_C(255)
-#define INT16_MIN (-INT16_MAX - 1)
-#define INT16_MAX 32767
-#define UINT16_MAX 65535u
+#define INT16_MIN (-INT16_MAX - INT16_C(1))
+#define INT16_MAX INT16_C(32767)
+#define UINT16_MAX UINT16_C(65535)
#ifdef __INT24_DEFINED
-# define INT24_MIN (-INT24_MAX - 1)
-# define INT24_MAX 8388607
-# define UINT24_MAX 16777215u
+# define INT24_MIN (-INT24_MAX - INT24_C(1))
Review comment:
ditto
##########
File path: include/stdint.h
##########
@@ -41,28 +41,28 @@
/* Limits of exact-width integer types */
-#define INT8_MIN (-INT8_MAX - 1)
-#define INT8_MAX 127
-#define UINT8_MAX 255
+#define INT8_MIN (-INT8_MAX - INT8_C(1))
Review comment:
or INT8_C(-128) directly
##########
File path: include/stdint.h
##########
@@ -41,28 +41,28 @@
/* Limits of exact-width integer types */
-#define INT8_MIN (-INT8_MAX - 1)
-#define INT8_MAX 127
-#define UINT8_MAX 255
+#define INT8_MIN (-INT8_MAX - INT8_C(1))
+#define INT8_MAX INT8_C(127)
+#define UINT8_MAX UINT8_C(255)
-#define INT16_MIN (-INT16_MAX - 1)
-#define INT16_MAX 32767
-#define UINT16_MAX 65535u
+#define INT16_MIN (-INT16_MAX - INT16_C(1))
+#define INT16_MAX INT16_C(32767)
+#define UINT16_MAX UINT16_C(65535)
#ifdef __INT24_DEFINED
-# define INT24_MIN (-INT24_MAX - 1)
-# define INT24_MAX 8388607
-# define UINT24_MAX 16777215u
+# define INT24_MIN (-INT24_MAX - INT24_C(1))
+# define INT24_MAX INT24_C(8388607)
+# define UINT24_MAX UINT24_C(16777215)
#endif
-#define INT32_MIN (-INT32_MAX - 1)
-#define INT32_MAX 2147483647
-#define UINT32_MAX 4294967295u
+#define INT32_MIN (-INT32_MAX - INT32_C(1))
+#define INT32_MAX INT32_C(2147483647)
+#define UINT32_MAX UINT32_C(4294967295)
#ifdef __INT64_DEFINED
-# define INT64_MIN (-INT64_MAX - 1ll)
-# define INT64_MAX 9223372036854775807ll
-# define UINT64_MAX 18446744073709551615ull
+# define INT64_MIN (-INT64_MAX - INT64_C(1))
Review comment:
ditto
##########
File path: include/stdint.h
##########
@@ -41,28 +41,28 @@
/* Limits of exact-width integer types */
-#define INT8_MIN (-INT8_MAX - 1)
-#define INT8_MAX 127
-#define UINT8_MAX 255
+#define INT8_MIN (-INT8_MAX - INT8_C(1))
+#define INT8_MAX INT8_C(127)
+#define UINT8_MAX UINT8_C(255)
-#define INT16_MIN (-INT16_MAX - 1)
-#define INT16_MAX 32767
-#define UINT16_MAX 65535u
+#define INT16_MIN (-INT16_MAX - INT16_C(1))
+#define INT16_MAX INT16_C(32767)
+#define UINT16_MAX UINT16_C(65535)
#ifdef __INT24_DEFINED
-# define INT24_MIN (-INT24_MAX - 1)
-# define INT24_MAX 8388607
-# define UINT24_MAX 16777215u
+# define INT24_MIN (-INT24_MAX - INT24_C(1))
+# define INT24_MAX INT24_C(8388607)
+# define UINT24_MAX UINT24_C(16777215)
#endif
-#define INT32_MIN (-INT32_MAX - 1)
-#define INT32_MAX 2147483647
-#define UINT32_MAX 4294967295u
+#define INT32_MIN (-INT32_MAX - INT32_C(1))
Review comment:
ditto
----------------------------------------------------------------
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 #2863: stdint.h: Use conversion macros for the definition of MIN and MAX constants
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2863:
URL: https://github.com/apache/incubator-nuttx/pull/2863#discussion_r578689145
##########
File path: include/stdint.h
##########
@@ -41,28 +41,28 @@
/* Limits of exact-width integer types */
-#define INT8_MIN (-INT8_MAX - 1)
-#define INT8_MAX 127
-#define UINT8_MAX 255
+#define INT8_MIN (-INT8_MAX - INT8_C(1))
Review comment:
Yes, you are right, my concern isn't valid here.
----------------------------------------------------------------
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] gustavonihei commented on a change in pull request #2863: stdint.h: Use conversion macros for the definition of MIN and MAX constants
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #2863:
URL: https://github.com/apache/incubator-nuttx/pull/2863#discussion_r578599141
##########
File path: include/stdint.h
##########
@@ -41,28 +41,28 @@
/* Limits of exact-width integer types */
-#define INT8_MIN (-INT8_MAX - 1)
-#define INT8_MAX 127
-#define UINT8_MAX 255
+#define INT8_MIN (-INT8_MAX - INT8_C(1))
Review comment:
Sorry, I couldn't follow. Which promotion do you expect to happen in this scenario?
I don't think it is really needed, since INTx_MAX should be already suffixed (if needed by the arch) and thus it won't be promoted for after the subtraction.
Regarding `-128`, I feel that making it relative to `INT8_MAX` definition copes with code comprehension.
And also it is the same approach used by LLVM:
https://clang.llvm.org/doxygen/stdint_8h_source.html
----------------------------------------------------------------
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] gustavonihei commented on a change in pull request #2863: stdint.h: Use conversion macros for the definition of MIN and MAX constants
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #2863:
URL: https://github.com/apache/incubator-nuttx/pull/2863#discussion_r578600026
##########
File path: include/stdint.h
##########
@@ -41,28 +41,28 @@
/* Limits of exact-width integer types */
-#define INT8_MIN (-INT8_MAX - 1)
-#define INT8_MAX 127
-#define UINT8_MAX 255
+#define INT8_MIN (-INT8_MAX - INT8_C(1))
Review comment:
Actually, I'll remove the INTx_C(1) since it has no effect to the end result.
----------------------------------------------------------------
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 #2863: stdint.h: Use conversion macros for the definition of MIN and MAX constants
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #2863:
URL: https://github.com/apache/incubator-nuttx/pull/2863
----------------------------------------------------------------
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] davids5 edited a comment on pull request #2863: stdint.h: Use conversion macros for the definition of MIN and MAX constants
Posted by GitBox <gi...@apache.org>.
davids5 edited a comment on pull request #2863:
URL: https://github.com/apache/incubator-nuttx/pull/2863#issuecomment-781556860
What is wrong with using simple to grock numbers that just make sense.?
```
# define INT8_MAX INT8_C(127)
# define INT8_MIN (-INT8_C(127)-1)
# define UINT8_MAX UINT8_C(255)
# define __INT_LEAST8_MIN INT8_MIN
# define __INT_LEAST8_MAX INT8_MAX
# define __UINT_LEAST8_MAX UINT8_MAX
```
----------------------------------------------------------------
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