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

NVStore.cpp compiler warning removal (os_ret) #10485

Merged
merged 1 commit into from May 3, 2019

Conversation

Projects
None yet
8 participants
@JanneKiiskila
Copy link
Contributor

commented Apr 25, 2019

Description

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.

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@davidsaada @bulislaw

NVStore.cpp compiler warning removal (os_ret)
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.

@ciarmcom ciarmcom requested review from bulislaw, davidsaada and ARMmbed/mbed-os-maintainers Apr 25, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

os_ret = flash_erase_area(area);
MBED_ASSERT(!os_ret);
ret = flash_erase_area(area);
MBED_ASSERT(!ret);

This comment has been minimized.

Copy link
@TeroJaasko

TeroJaasko Apr 26, 2019

Contributor

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?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Apr 26, 2019

Contributor

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.

This comment has been minimized.

Copy link
@JanneKiiskila

JanneKiiskila Apr 26, 2019

Author Contributor

Tend to agree here. :-) But, well out of scope for this PR in a way.

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Apr 29, 2019

Member

Tend to agree here. :-) But, well out of scope for this PR in a way.

Please create a tracking issue for this.

This comment has been minimized.

Copy link
@JanneKiiskila

JanneKiiskila Apr 29, 2019

Author Contributor

Done, IOTSTOR-832

This comment has been minimized.

Copy link
@TeroJaasko

TeroJaasko Apr 29, 2019

Contributor

@kjbracey-arm at least some drivers seem to notify HW/driver problems, not just user errors:

ret = flash_erase_sector(&_flash, addr);

->
int32_t flash_erase_sector(flash_t *obj, uint32_t address)
{
(void)(obj);
int32_t status = 0;
if (Cy_Flash_EraseRow(address) != CY_FLASH_DRV_SUCCESS) {
status = -1;
}
return status;
}

@JanneKiiskila

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

@JanneKiiskila

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

@adbridge @0xc0170 - can we merge this one in, please? One warning less, future work saved to backlog.

@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

@TeroJaasko @kjbracey-arm are you happy for this to go in as is ?

@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

@JanneKiiskila has to be approved by the reviewers and go through CI successfully before it can be 'merged' in :)

@0xc0170 0xc0170 added needs: CI and removed needs: review labels May 2, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 2, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented May 2, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit 3eaad5f into ARMmbed:master May 3, 2019

26 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage Success
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8622 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details

@0xc0170 0xc0170 removed the ready for merge label May 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.