2 cosmetic and minor improvement patches #10187
Conversation
lyakh
requested review from
bardliao,
dbaluta,
jxstelter,
kv2019i,
lbetlej,
lgirdwood,
marcinszkudlinski,
mmaka1,
pblaszko,
plbossart and
ranj063
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR contains minor cosmetic code improvements across three audio processing files, focusing on code cleanup and documentation fixes.
- Removes an unreachable return statement in IPC helper code
- Fixes variable declaration order in module adapter preparation
- Corrects a typo in documentation and simplifies a list iteration
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ipc/ipc-helper.c | Removes unreachable return statement after conditional branch |
| src/audio/module_adapter/module_adapter.c | Moves variable declaration outside of loop for better scoping |
| src/audio/module_adapter/module/generic.c | Fixes typo in comment and simplifies list iteration by removing unused safe iterator |
Comments suppressed due to low confidence (1)
src/ipc/ipc-helper.c:178
- Removing this return statement changes the function's behavior. The code after the if-else block will now execute when there are no connected buffers, which may not be the intended logic. Verify that this change is correct and that the subsequent code should run in the 'no connected buffers' case.
} else {
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| /** | ||
| * Allocates aligned memory block for module. | ||
| * @param mod Pointer to the module this memory block is allocatd for. | ||
| * @param mod Pointer to the module this memory block is allocated for. |
There was a problem hiding this comment.
Fixed typo: 'allocatd' corrected to 'allocated'.
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Not fully agreeing with all the changes, but net positive for the series.
| return 0; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
This is first patch is really not just a cosmetic fix.
| size_t size = MAX(mod->deep_buff_bytes, mod->period_bytes); | ||
| size_t size = MAX(mod->deep_buff_bytes, mod->period_bytes); | ||
|
|
||
| list_for_item(blist, &dev->bsource_list) { |
There was a problem hiding this comment.
Hmm, compiler should optimize that out? This does bring "size" to the wider scope, so not sure if this is better. Not blocking...
There was a problem hiding this comment.
@kv2019i I'm not sure the compiler is able to deduce that mod->deep_buff_bytes and mod->period_bytes don't change while the loop is executed. So I wouldn't rely on that
The size calculation in module_adapter_prepare() is constant over all loop iterations, move it out of the loop. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Remove a redundant return statement. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
lyakh
changed the title
|
@lyakh can you check CI thanks ! |
@lgirdwood https://sof-ci.01.org/sofpr/PR10187/build15460/devicetest/index.html?model=MTLP_RVP_NOCODEC&testcase=multiple-pause-resume-50 is a known MTL failure, https://sof-ci.01.org/sofpr/PR10187/build15458/devicetest/index.html?model=LNLM_RVP_NOCODEC&testcase=multiple-pause-resume-50 looks identical to it too. |
4 participants
No description provided.