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 2022/10/31 18:14:46 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request, #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
xiaoxiang781216 opened a new pull request, #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491
## Summary
## Impact
## Testing
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1009737626
##########
net/sixlowpan/sixlowpan_internal.h:
##########
@@ -103,15 +103,15 @@
/* GET 16-bit data: source in network order */
#define GETUINT16(ptr,index) \
- ((((uint16_t)((ptr)[index])) << 8) | ((uint16_t)(((ptr)[(index) + 1]))))
+ ((uint16_t)(((ptr)[index] << 8) | (ptr)[(index) + 1]))
/* PUT 16-bit data: source in host order, result in network order */
#define PUTHOST16(ptr,index,value) \
do \
{ \
- (ptr)[index] = ((uint16_t)(value) >> 8) & 0xff; \
- (ptr)[index + 1] = (uint16_t)(value) & 0xff; \
+ (ptr)[index] = ((value) >> 8) & 0xff; \
Review Comment:
ditto
##########
net/sixlowpan/sixlowpan_internal.h:
##########
@@ -103,15 +103,15 @@
/* GET 16-bit data: source in network order */
#define GETUINT16(ptr,index) \
- ((((uint16_t)((ptr)[index])) << 8) | ((uint16_t)(((ptr)[(index) + 1]))))
+ ((uint16_t)(((ptr)[index] << 8) | (ptr)[(index) + 1]))
Review Comment:
what is the warning here?
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
what is the warning 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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#issuecomment-1297500123
Let's sue relative path in first 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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1010087395
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
Yes, variable parameters list or expression will promote uint16_t to int, but it isn't related to this problem. GETUINT16 should ensure the result type is uint16_t regardless the promotion in the following evaluation.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1010451252
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((uint16_t)(p[1] << 8) | p[0]))
Review Comment:
Done
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1009798993
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
> is this a place of warning?
>
> ```
> uinfo("ECM_SET_PACKET_FILTER wValue: 0x%04hx, wIndex: 0x%04hx\n",
> GETUINT16(ctrl->value), GETUINT16(ctrl->index));
> ```
>
> If yes, then also good to use `PRIx16` instead of `hx`. All NuttX ports have `#define PRIx16 "x"`.
Yes, but the root cause is that the return value of GETUINT16 is `int` not `uint16_t`(`unsigned short`), because compiler promote all integer smaller than `int` to `int`. The right fix is move the cast to the last.
> I need to refresh in my memory how shift works on 16bit CPUs. For 32-bit and 64-bit there will be no issues for sure, but I'm a bit concerned if change of `GETUINT32` will produce the same output when run on 16-bit CPU. I mean if argument is extended to CPU word, then `p[3] << 24` and `(uint32_t)p[3] << 24` might result to a different values, but I need to check if that is true
Ok, I restore GETUINT32 since it doesn't suffer the integer promotion.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1009762969
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
is this a place of warning?
```
uinfo("ECM_SET_PACKET_FILTER wValue: 0x%04hx, wIndex: 0x%04hx\n",
GETUINT16(ctrl->value), GETUINT16(ctrl->index));
```
If yes, then also good to use `PRIx16` instead of `hx`. All NuttX ports have `#define PRIx16 "x"`.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1009798993
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
> is this a place of warning?
>
> ```
> uinfo("ECM_SET_PACKET_FILTER wValue: 0x%04hx, wIndex: 0x%04hx\n",
> GETUINT16(ctrl->value), GETUINT16(ctrl->index));
> ```
>
> If yes, then also good to use `PRIx16` instead of `hx`. All NuttX ports have `#define PRIx16 "x"`.
Yes, but the root cause is that the return value of GETUINT16 should be uint16_t(unsigned short) not int, because compiler promote all integer smaller than `int` to `int`. The right fix is move the cast to the last.
> I need to refresh in my memory how shift works on 16bit CPUs. For 32-bit and 64-bit there will be no issues for sure, but I'm a bit concerned if change of `GETUINT32` will produce the same output when run on 16-bit CPU. I mean if argument is extended to CPU word, then `p[3] << 24` and `(uint32_t)p[3] << 24` might result to a different values, but I need to check if that is true
Ok, I restore GETUINT32 since it doesn't suffer the integer promotion.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1009744271
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
Here:
```
Fix error: format specifies type 'unsigned short' but the argument has type 'int'
```
The error message is included in the commit message, please review the change patch by patch.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1010082175
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
I think that compiler warning is a bit redundant and `uint16_t` parameter anyway will be promoted to `int` during the function call. Let me experiment a bit with variable parameters list a bit and I will come back with a wider answer. I need to renew things in memory by running some test code.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#issuecomment-1298047110
@xiaoxiang781216 please update commit message in the first commit to use relative file path or just a file name
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1009765986
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
I need to refresh in my memory how shift works on 16bit CPUs. For 32-bit and 64-bit there will be no issues for sure, but I'm a bit concerned if change of `GETUINT32` will produce the same output when run on 16-bit CPU. I mean if argument is extended to CPU word, then `p[3] << 24` and `(uint32_t)p[3] << 24` might result to a different values, but I need to check if that is true
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#issuecomment-1297983190
> > Testing
> > Pass CI
>
> @xiaoxiang781216 Hmm, the CI is still failing. How did you test this PR?
This PR try to fix the warning reported by clang, all changes are minor fix without the functionality change, so all CI pass is good to verify the change is good.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1010090788
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
let's just place type cast in front of return result as in
```
# define __swap_uint32(n) \
(uint32_t)(((((uint32_t)(n)) & 0x000000ffUL) << 24) | \
((((uint32_t)(n)) & 0x0000ff00UL) << 8) | \
((((uint32_t)(n)) & 0x00ff0000UL) >> 8) | \
((((uint32_t)(n)) & 0xff000000UL) >> 24))
```
Do you agree?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#issuecomment-1312630702
https://github.com/apache/incubator-nuttx/pull/7391
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1010089765
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
> Yes, variable parameters list or expression will promote uint16_t to int, but it isn't related to this problem. GETUINT16 should ensure the result type is uint16_t regardless whether the promotion happen in the following evaluation.
How do you ensure that? I can bet that the assembly code will put that value into a register. How can you ensure that register will be 16 bit long on 32 bit machine?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1010087395
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
Yes, variable parameters list or expression will promote uint16_t to int, but it isn't related to this problem. GETUINT16 should ensure the result type is uint16_t regardless whether the promotion happen in the following evaluation.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1009798993
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
> is this a place of warning?
>
> ```
> uinfo("ECM_SET_PACKET_FILTER wValue: 0x%04hx, wIndex: 0x%04hx\n",
> GETUINT16(ctrl->value), GETUINT16(ctrl->index));
> ```
>
> If yes, then also good to use `PRIx16` instead of `hx`. All NuttX ports have `#define PRIx16 "x"`.
Yes, but the root cause is that the return type of GETUINT16 is `int` not `uint16_t`(`unsigned short`), because compiler promote all integer smaller than `int` to `int`. The right fix is move the cast to the last operation.
> I need to refresh in my memory how shift works on 16bit CPUs. For 32-bit and 64-bit there will be no issues for sure, but I'm a bit concerned if change of `GETUINT32` will produce the same output when run on 16-bit CPU. I mean if argument is extended to CPU word, then `p[3] << 24` and `(uint32_t)p[3] << 24` might result to a different values, but I need to check if that is true
Ok, I restore GETUINT32 since it doesn't suffer the integer promotion.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1009765986
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
I need to refresh in my memory how shift works on 16bit CPUs. For 32-bit and 64-bit there will be no issues for sure, but I'm a bit concerned if change of `GETUINT32` will produce the same output when run on 16-bit CPU. I mean if argument is extended to CPU word, then `p[3] << 24` and `(uint32_t)p[3] << 24` might result to a different values
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko merged pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1009744271
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
Here(https://github.com/apache/incubator-nuttx/pull/7491/commits/0f75bf7389ddc09718920b78b1868b4a8e57530c):
```
Fix error: format specifies type 'unsigned short' but the argument has type 'int'
```
The error message is included in the commit message, please review the change patch by patch.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#issuecomment-1297915314
>Testing
>Pass CI
@xiaoxiang781216
Hmm, the CI is still failing. How did you test this PR?
```
====================================================================================
Configuration/Tool: zkit-arm-1769/nxhello,CONFIG_ARM_TOOLCHAIN_GNU_EABI
------------------------------------------------------------------------------------
Cleaning...
Configuring...
Disabling CONFIG_ARM_TOOLCHAIN_BUILDROOT_OABI
Enabling CONFIG_ARM_TOOLCHAIN_GNU_EABI
Building NuttX...
chip/lpc17_40_serial.c: In function 'arm_earlyserialinit':
Error: chip/lpc17_40_serial.c:1602:25: error: implicit declaration of function 'lpc17_40_uartcclkdiv'; did you mean 'lpc17_40_uartdl'? [-Werror=implicit-function-declaration]
1602 | g_uart0priv.cclkdiv = lpc17_40_uartcclkdiv(CONFIG_UART0_BAUD);
| ^~~~~~~~~~~~~~~~~~~~
| lpc17_40_uartdl
cc1: all warnings being treated as errors
make[1]: *** [Makefile:144: lpc17_40_serial.o] Error 1
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1010088983
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
Try running
```
#include <stdio.h>
#include <stdint.h>
#define MK16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
#define MK16_(p) ((uint16_t)((p[1] << 8) | p[0]))
int main()
{
uint8_t a[2] = {1,2};
uint8_t b[2] = {3,4};
printf("Output1 %04hx %04hx\n", MK16(a), MK16(b));
printf("Output2 %04hx %04hx\n", MK16_(a), MK16_(b));
return 0;
}
```
Both will print
```
Output1 0201 0403
Output2 0201 0403
```
If the warning matters then I was expecting to see `0000` printed as a second value of a faulty case.
The `hx` matters only for `scanf` case and that is why `PRIx8` and `PRIx16` are defined to `"x"`.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1009798993
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
> is this a place of warning?
>
> ```
> uinfo("ECM_SET_PACKET_FILTER wValue: 0x%04hx, wIndex: 0x%04hx\n",
> GETUINT16(ctrl->value), GETUINT16(ctrl->index));
> ```
>
> If yes, then also good to use `PRIx16` instead of `hx`. All NuttX ports have `#define PRIx16 "x"`.
Yes, but the root cause is that the return type of GETUINT16 is `int` not `uint16_t`(`unsigned short`), because compiler promote all integer smaller than `int` to `int`. The right fix is move the cast to the last.
> I need to refresh in my memory how shift works on 16bit CPUs. For 32-bit and 64-bit there will be no issues for sure, but I'm a bit concerned if change of `GETUINT32` will produce the same output when run on 16-bit CPU. I mean if argument is extended to CPU word, then `p[3] << 24` and `(uint32_t)p[3] << 24` might result to a different values, but I need to check if that is true
Ok, I restore GETUINT32 since it doesn't suffer the integer promotion.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1009798993
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
> is this a place of warning?
>
> ```
> uinfo("ECM_SET_PACKET_FILTER wValue: 0x%04hx, wIndex: 0x%04hx\n",
> GETUINT16(ctrl->value), GETUINT16(ctrl->index));
> ```
>
> If yes, then also good to use `PRIx16` instead of `hx`. All NuttX ports have `#define PRIx16 "x"`.
Yes, but the root cause is that the return value of GETUINT16 should be `uint16_t`(`unsigned short`) not `int`, because compiler promote all integer smaller than `int` to `int`. The right fix is move the cast to the last.
> I need to refresh in my memory how shift works on 16bit CPUs. For 32-bit and 64-bit there will be no issues for sure, but I'm a bit concerned if change of `GETUINT32` will produce the same output when run on 16-bit CPU. I mean if argument is extended to CPU word, then `p[3] << 24` and `(uint32_t)p[3] << 24` might result to a different values, but I need to check if that is true
Ok, I restore GETUINT32 since it doesn't suffer the integer promotion.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1010090270
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((p[1] << 8) | p[0]))
Review Comment:
I mean that `GETUINT32` will be placed into 64 bit register on 64 bit machine.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#issuecomment-1298052663
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1010303768
##########
include/nuttx/usb/usb.h:
##########
@@ -65,10 +65,10 @@
#define MSBYTE(u16) ((u16) >> 8) /* Get MS byte from uint16_t */
#define LSBYTE(u16) ((u16) & 0xff) /* Get LS byte from uint16_t */
-#define GETUINT16(p) (((uint16_t)p[1] << 8) | (uint16_t)p[0])
-#define GETUINT32(p) (((uint32_t)p[3] << 24) | \
- ((uint32_t)p[2] << 16) | \
- ((uint32_t)p[1] << 8) | (uint32_t)p[0])
+#define GETUINT16(p) ((uint16_t)((uint16_t)(p[1] << 8) | p[0]))
Review Comment:
Please get back the original uint16_t cast
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #7491: Fix the warning found by https://github.com/apache/incubator-nuttx/pull/7391
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #7491:
URL: https://github.com/apache/incubator-nuttx/pull/7491#discussion_r1009744969
##########
net/sixlowpan/sixlowpan_internal.h:
##########
@@ -103,15 +103,15 @@
/* GET 16-bit data: source in network order */
#define GETUINT16(ptr,index) \
- ((((uint16_t)((ptr)[index])) << 8) | ((uint16_t)(((ptr)[(index) + 1]))))
+ ((uint16_t)(((ptr)[index] << 8) | (ptr)[(index) + 1]))
Review Comment:
Ack.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org