xiaoxiang781216 commented on code in PR #1114:
URL:
https://github.com/apache/incubator-nuttx-apps/pull/1114#discussion_r842789859
##########
system/sched_note/note_main.c:
##########
@@ -83,6 +83,36 @@ static FAR const char *g_statenames[] =
* Private Functions
************************************************************************************/
+/************************************************************************************
+ * Name: trace_dump_unflatten
+
************************************************************************************/
+
+static void trace_dump_unflatten(FAR void *dst,
+ FAR uint8_t *src, size_t len)
+{
+ switch (len)
+ {
+#ifdef CONFIG_HAVE_LONG_LONG
+ case 8:
+ *(uint64_t *)dst = ((uint64_t)src[7] << 56)
+ + ((uint64_t)src[6] << 48)
+ + ((uint64_t)src[5] << 40)
+ + ((uint64_t)src[4] << 32);
+#endif
+ case 4:
+ *(uint32_t *)dst = ((uint64_t)src[3] << 24)
+ + ((uint64_t)src[2] << 16);
+ case 2:
+ *(uint16_t *)dst = ((uint64_t)src[1] << 8);
+ case 1:
+ *(uint8_t *)dst = src[0];
+ break;
Review Comment:
> This code seems to be totally wrong from many perspectives. We need to use
temporary variable here and memcpy at the end:
>
> ```
> #ifdef CONFIG_HAVE_LONG_LONG
> uint64_t tmp = 0;
> #else
> uint32_t tmp = 0;
> #endif
>
> switch (len)
> {
> #ifdef CONFIG_HAVE_LONG_LONG
> case 8:
> tmp += ((uint64_t)src[7] << 56);
> tmp += ((uint64_t)src[6] << 48);
> tmp += ((uint64_t)src[5] << 40);
> tmp += ((uint64_t)src[4] << 32);
> #endif
> case 4:
> tmp += ((uint32_t)src[3] << 24);
> tmp += ((uint32_t)src[2] << 16);
> case 2:
> tmp += ((uint16_t)src[1] << 8);
> case 1:
> tmp += src[0];
> break;
> default:
> DEBUGASSERT(FALSE);
> break;
> }
>
> memcpy(dst, &tmp, len);
> ```
>
it is no difference from cast the dest to uint64_t * and assign the value
directly.
> In this way we will keep data in host byte order. But I'm not sure if that
is the goal.
The code assume the source is in little endian and convert to the host
endian.
> If I take a look into a scheduler variant it produce little endian always:
>
> ```
> static inline void sched_note_flatten(FAR uint8_t *dst,
> FAR void *src, size_t len)
> {
> switch (len)
> {
> #ifdef CONFIG_HAVE_LONG_LONG
> case 8:
> dst[7] = (uint8_t)((*(uint64_t *)src >> 56) & 0xff);
> dst[6] = (uint8_t)((*(uint64_t *)src >> 48) & 0xff);
> dst[5] = (uint8_t)((*(uint64_t *)src >> 40) & 0xff);
> dst[4] = (uint8_t)((*(uint64_t *)src >> 32) & 0xff);
> #endif
> case 4:
> dst[3] = (uint8_t)((*(uint32_t *)src >> 24) & 0xff);
> dst[2] = (uint8_t)((*(uint32_t *)src >> 16) & 0xff);
> case 2:
> dst[1] = (uint8_t)((*(uint16_t *)src >> 8) & 0xff);
> case 1:
> dst[0] = (uint8_t)(*(uint8_t *)src & 0xff);
> break;
> default:
> DEBUGASSERT(FALSE);
> break;
> }
> }
> ```
Yes, the dest is in the little endian.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
