Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

spinlock: use inline replace macro #14932

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

spinlock: use inline replace macro #14932

wants to merge 1 commit into from

Conversation

hujun260
Copy link
Contributor

Summary

spinlock: use inline replase macro

reason:
we.avoid.use gcc express statement extension in spinlock, to enhance compatibility

Impact

spinlock

Testing

ci ostest

@github-actions github-actions bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Nov 25, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 25, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and mentions testing, it lacks crucial details.

Here's a breakdown of what's missing:

  • Summary: While it states the change ("use inline replace macro"), the why is vague ("enhance compatibility"). Which compilers are problematic? What specific compatibility issues are being addressed? How does an inline macro solve this? Links to related issues are missing.
  • Impact: The impact section is far too brief. It just mentions "spinlock" which doesn't tell us anything. All the "NO/YES" questions need to be answered explicitly. Even if the answer is NO, it should be stated. For example: "Impact on user: NO". If there are any YES answers, a description is required.
  • Testing: "ci ostest" is insufficient. What platform was this tested on (host and target)? Which compiler version? What do "ostest" results actually show? The requirement specifically asks for "Testing logs before change" and "Testing logs after change". Simply saying "ci ostest" doesn't demonstrate what changed or that the change was successful. Provide concrete examples of the problematic behavior before the change and how it's resolved after the change.

The PR needs to be significantly more detailed to meet the NuttX requirements. Provide specific information for each section to demonstrate a thorough understanding of the change and its impact.

anchao
anchao requested changes Nov 25, 2024
View reviewed changes
Copy link
Contributor

@anchao anchao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you currently using a compiler that does not support this extension? If not, please keep this implementation, otherwise !Please delete all the code in nuttx that uses this extended syntax

@acassis acassis changed the title spinlock: use inline replase macro spinlock: use inline replace macro Nov 25, 2024
xiaoxiang781216
xiaoxiang781216 reviewed Nov 25, 2024
View reviewed changes
include/nuttx/spinlock.h Outdated
})
static inline_function
bool spin_trylock_irqsave_wo_note(FAR volatile spinlock_t *lock,
irqstate_t *flags)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
irqstate_t *flags)
FAR irqstate_t *flags)

include/nuttx/spinlock.h Outdated
})
static inline_function
bool spin_trylock_irqsave_wo_note(FAR volatile spinlock_t *lock,
irqstate_t *flags)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
irqstate_t *flags)
FAR irqstate_t *flags)

include/nuttx/spinlock.h Outdated
true : ({ up_irq_restore(f); false; }); \
})
static inline_function
bool spin_trylock_irqsave(FAR volatile spinlock_t *lock, irqstate_t *flags)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool spin_trylock_irqsave(FAR volatile spinlock_t *lock, irqstate_t *flags)
bool spin_trylock_irqsave(FAR volatile spinlock_t *lock, FAR irqstate_t *flags)

include/nuttx/spinlock.h Outdated
true; \
})
static inline_function
bool spin_trylock_irqsave(FAR volatile spinlock_t *lock, irqstate_t *flags)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool spin_trylock_irqsave(FAR volatile spinlock_t *lock, irqstate_t *flags)
bool spin_trylock_irqsave(FAR volatile spinlock_t *lock, FAR irqstate_t *flags)

@hujun260
spinlock: use inline replace macro
5e23c4a 
reason:
we.avoid.use gcc express statement extension in spinlock, to enhance compatibility

Signed-off-by: hujun5 <[email protected]>
@hujun260
Copy link
Contributor Author

spin_trylock_irqsave

OK， Keep it as it is.

@hujun260 hujun260 marked this pull request as draft November 25, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaoxiang781216 xiaoxiang781216 xiaoxiang781216 left review comments

@anchao anchao anchao requested changes

@acassis acassis acassis approved these changes

Assignees
No one assigned
Labels
Area: OS Components OS Components issues Size: S The size of the change in this PR is small
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hujun260 @nuttxpr @acassis @anchao @xiaoxiang781216