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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
