Skip to content

Conversation

@hujun260
Copy link
Contributor

@hujun260 hujun260 commented Dec 4, 2024

Summary

remove redundant judgments *running_task != NULL
reason:
In irq, g_running_tasks is always valid.

Impact

arch

Testing

ci ostest

@github-actions github-actions bot added Arch: avr Issues related to all AVR(8-bit or 32-bit) architectures Arch: hc Issues related to HC architecture Arch: openrisc Issues related to the OpenRISC architecture Arch: renesas Issues related to the Renesas chips Arch: simulator Issues related to the SIMulator Arch: x86 Issues related to the x86 architecture Arch: x86_64 Issues related to the x86_64 architecture Arch: xtensa Issues related to the Xtensa architecture Arch: z16 Issues related to the Z16 architecture Arch: z80 Issues related to the Z80 architecture Size: S The size of the change in this PR is small labels Dec 4, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 4, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and identifies the impacted area, it lacks crucial information.

Here's what's missing:

  • Insufficient Summary: The summary needs to explain why the judgments are redundant. Simply stating they are redundant isn't enough. What guarantees g_running_task is always valid in an IRQ? Provide a code reference or architectural explanation. What function/file contains this change?
  • Incomplete Impact: While it mentions "arch," it needs to specify which architectures are affected. Furthermore, it needs to address all the impact points (user, build, hardware, documentation, security, compatibility). Even if the answer is "NO," it should explicitly state that.
  • Missing Testing Details: "ci ostest" is not sufficient. What specific tests were run? Provide details about the build host and target environment (OS, architecture, board, configuration). Crucially, the "Testing logs before change" and "Testing logs after change" sections are empty. These should demonstrate the problem before the change and the successful resolution after the change.

In short, the PR needs significantly more detail to meet the NuttX requirements. It needs to provide a clear and compelling justification for the change, thoroughly analyze its impact, and offer concrete evidence that the change works as intended.

reason:
In irq, g_running_tasks is always valid.

Signed-off-by: hujun5 <hujun5@xiaomi.com>
@xiaoxiang781216
Copy link
Contributor

let's ignore the ci error since it's fixed by #15047:

In file included from bluetooth/bt_conn.c:51:
bluetooth/bt_conn.c: In function 'bt_conn_addref':
Error: bluetooth/bt_conn.c:765:10: error: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'bt_atomic_t' {aka 'long int'} [-Werror=format=]
  765 |   wlinfo("handle %u ref %u\n", conn->handle, bt_atomic_get(&conn->ref));
      |          ^~~~~~~~~~~~~~~~~~~~
bluetooth/bt_conn.c:765:26: note: format string is defined here
  765 |   wlinfo("handle %u ref %u\n", conn->handle, bt_atomic_get(&conn->ref));
      |                         ~^
      |                          |
      |                          unsigned int
      |                         %lu
bluetooth/bt_conn.c: In function 'bt_conn_release':
Error: bluetooth/bt_conn.c:790:10: error: format '%u' expects argument of type 'unsigned int', but argument 4 has type 'bt_atomic_t' {aka 'long int'} [-Werror=format=]
  790 |   wlinfo("handle %u ref %u\n", conn->handle, bt_atomic_get(&conn->ref));
      |          ^~~~~~~~~~~~~~~~~~~~
bluetooth/bt_conn.c:790:26: note: format string is defined here
  790 |   wlinfo("handle %u ref %u\n", conn->handle, bt_atomic_get(&conn->ref));
      |                         ~^
      |                          |
      |                          unsigned int
      |                         %lu

@xiaoxiang781216 xiaoxiang781216 merged commit dabf589 into apache:master Dec 4, 2024
17 of 27 checks passed
@hujun260 hujun260 deleted the apache_50 branch December 5, 2024 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: avr Issues related to all AVR(8-bit or 32-bit) architectures Arch: hc Issues related to HC architecture Arch: openrisc Issues related to the OpenRISC architecture Arch: renesas Issues related to the Renesas chips Arch: simulator Issues related to the SIMulator Arch: x86 Issues related to the x86 architecture Arch: x86_64 Issues related to the x86_64 architecture Arch: xtensa Issues related to the Xtensa architecture Arch: z16 Issues related to the Z16 architecture Arch: z80 Issues related to the Z80 architecture Size: S The size of the change in this PR is small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants