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.

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

Initial SMP review - DO NOT MERGE #750

Closed
wants to merge 1 commit into from

Conversation

RichardBarry
Copy link
Contributor

Description

DO NOT MERGE THIS PR – it is only intended as a code review

This PR contains multiple “_RB_” tags in comments highlighting review comments I left in the code. Some are trivial and just stylistic, for example, where macro names are not consistent with the kernel’s style guide. More fundamentally though, I would like us to experiment with reducing use of the pre-processor to split the code into single and multi-core versions. I feel this can be done by:

  1. Using the same pxCurrentTCB[] array no matter the setting of configNUMBER_OF_CORES, just as the same xYieldPends[] array is already used in all cases. The loops through the pxCurrentTCB[] array should still work even when there is only one position in the array.
  2. Implementing single core versions of the macros that currently only exist when configNUMBER_OF_CORES > 1. Hopefully some of these can be defined as constants, or even just defined as nothing, when configNUMBER_OF_CORES == 1 to reduce code size impact. If successful, this will reduce duplication, testing and maintenance, and improve readability. Currently single core configurations are not exercising much of the same code as multicore configurations.
    So far, I’ve reviewed the first quarter of the file to prevent adding the same comment in multiple places, and not commented on design.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@RichardBarry RichardBarry requested a review from a team as a code owner August 13, 2023 21:09
@codecov
Copy link

codecov bot commented Aug 13, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (6f35865) 94.35% compared to head (814f078) 94.35%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #750   +/-   ##
=======================================
  Coverage   94.35%   94.35%           
=======================================
  Files           6        6           
  Lines        2443     2443           
  Branches      598      598           
=======================================
  Hits         2305     2305           
  Misses         85       85           
  Partials       53       53           
Flag Coverage Δ
unittests 94.35% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
tasks.c 94.82% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chinglee-iot
Copy link
Member

chinglee-iot commented Oct 18, 2023

Commits address the review feedback in this PR are merged into main branch. Let's close this PR and use new PR for other suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants