NVStore.cpp compiler warning removal (os_ret) #10485
Conversation
One gets this compiler warning from nvstore.cpp: ``` Compile [ 48.6%]: nvstore.cpp [Warning] nvstore.cpp@814,9: variable 'os_ret' set but not used [-Wunused-but-set-variable] ``` Turns out it's caused by the fact that the variable is only used with MBED_ASSERTs, which get optimized out or not, depending on your build profile. In reality we do not need a separate variable for that in my opinion though, so we can just use the ret-variable instead and drop the os_ret variable completely and thus avoid this compiler warning.
|
@JanneKiiskila, thank you for your changes. |
| os_ret = flash_erase_area(area); | ||
| MBED_ASSERT(!os_ret); | ||
| ret = flash_erase_area(area); | ||
| MBED_ASSERT(!ret); |
There was a problem hiding this comment.
It is slightly worrying that the errors due to HW problems are detected only on debug-builds. If the remaining code survives from storage error, what is the point of assert here?
There was a problem hiding this comment.
Do we expect hardware problems to be indicated by error returns from flash functions though?
It's quite possible that the only error returns would be for parameter errors like address being out of range, and we only find out flash failure by actually trying to use it and finding it doesn't hold data correctly.
But I would agree that if these functions can indicate hardware problems, there should be paths to deal with that. Asserts should be reserved for catching should-be-impossible design errors.
There was a problem hiding this comment.
Tend to agree here. :-) But, well out of scope for this PR in a way.
There was a problem hiding this comment.
Tend to agree here. :-) But, well out of scope for this PR in a way.
Please create a tracking issue for this.
There was a problem hiding this comment.
Done, IOTSTOR-832
There was a problem hiding this comment.
@kjbracey-arm at least some drivers seem to notify HW/driver problems, not just user errors:
Line 204 in cc94690
->
mbed-os/targets/TARGET_Cypress/TARGET_PSOC6/flash_api.c
Lines 41 to 50 in d2e9fde
|
@TeroJaasko @kjbracey-arm are you happy for this to go in as is ? |
|
@JanneKiiskila has to be approved by the reviewers and go through CI successfully before it can be 'merged' in :) |
|
CI started |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
Description
One gets this compiler warning from nvstore.cpp:
Turns out it's caused by the fact that the variable is only used
with
MBED_ASSERTs, which get optimized out or not, depending on yourbuild profile. In reality we do not need a separate variable for that
in my opinion though, so we can just use the
ret-variable insteadand drop the
os_retvariable completely and thus avoid thiscompiler warning.
Reviewers
@davidsaada @bulislaw