Fix Coverity issue: Initialize FlashIAP non-static member in constructor

[hugueskamba]

Description

Issue fixed:
FlashIAP.cpp:52 Non-static class member field _flash.dummy is not initialized in this constructor nor in any functions that it calls.

Pull request type

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

Reviewers

Release Notes

[ciarmcom]

@hugueskamba, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

[kjbracey]

To some extent, addressing some of these "uninitialized" warnings may add code size without actually changing anything - structures like that don't need to be initialised.

So I'd be interested in making changes at the same time to potentially trim out some code to balance it. Here you could remove the empty destructor, and make the constructor inline by putting it in the class body (it could also be constexpr, given that the real init happens later).

[hugueskamba]

To some extent, addressing some of these "uninitialized" warnings may add code size without actually changing anything - structures like that don't need to be initialised.

So I'd be interested in making changes at the same time to potentially trim out some code to balance it. Here you could remove the empty destructor, and make the constructor inline by putting it in the class body (it could also be constexpr, given that the real init happens later).

I get the following error if constexpr is used.

./mbed-os/drivers/FlashIAP.h:66:15: note: non-constexpr constructor 'NonCopyable' cannot be used in a constant expression
./mbed-os/platform/NonCopyable.h:175:5: note: declared here
    NonCopyable() { }

[kjbracey]

Hmm, NonCopyable is kind of in need of some C++11 loving. At a minimum, just changing that to

 NonCopyable() = default;
 ~NonCopyable() = default;

should silence that error without really changing anything else about it.

[hugueskamba]

Hmm, NonCopyable is kind of in need of some C++11 loving. At a minimum, just changing that to

 NonCopyable() = default;
 ~NonCopyable() = default;

should silence that error without really changing anything else about it.

The code size is no different before my change, after my change or with the changes you are suggesting. I will leave as is because there is no meaningful benefits in making more changes.

[kjbracey]

The code size is no different before my change, after my change or with the changes you are suggesting.

Are you sure you're building an image that actually uses FlashIAP somewhere? (And all toolchains?)

I can believe the constructor changes might have no effect in some or even all toolchains, but I'm confident the destructor removal really must make a code size difference, at least the way FlashIAP is used in nvstore and kvstore.

[hugueskamba]

The code size is no different before my change, after my change or with the changes you are suggesting.

Are you sure you're building an image that actually uses FlashIAP somewhere? (And all toolchains?)

I can believe the constructor changes might have no effect in some or even all toolchains, but I'm confident the destructor removal really must make a code size difference, at least the way FlashIAP is used in nvstore and kvstore.

Good point...I am using k64f. How can I find out if a target uses FlashIAP? Is it from targets/targets.json? If so, what should I be looking for?

[kjbracey]

It'll need to be a combo of a target and an application that uses its flash for storage. Sorry, I'm not really an expert. You'd want something like https://github.com/ARMmbed/mbed-os-example-kvstore for a real example.

It might be easier just to have a dummy test program - add this to blinky:

static void my_flash_test()
{
    FlashIAP flash;
}

main()
{
     my_flash_test();
}

That runs the construction/destruction cycle like kvstore does, albeit in a rather synthetic context.

(Possibly too synthetic - it might overoptimise the new form, realising it doesn't even need to bother doing any construction. Might want to add a flash.init(). That will work on K64F)

[hugueskamba]

This force-push removes FlashIAP destructor and inlines the constructor. It also uses C++ syntax in NonCopyable.h to avoid an error caused by the use of constexpr for FlashIAP constructor.

[kjbracey]

Just going to mention in passing that C++11 gives you another option here, which is kind of neater, but maybe less familiar.

You could do

// no need to declare constructor (it will be automatically constexpr)

private:
    flash_t _flash{};
    uint8_t *_page_buf = nullptr;

C++ Core Guidelines recommends that (see C.48 and C.49), but obviously that syntax is unfamiliar in our tree, what with us only just switching to C++11/14.

[0xc0170]

I think we should first update our guidelines and then do this for the codebase (good mention !)

[evedon]

@hugueskamba what is the difference in code size before and after this PR with the dummy test program?

[hugueskamba]

@hugueskamba what is the difference in code size before and after this PR with the dummy test program?

Using Blinky with the changes suggested by Kevin in main.cpp and the ARM toolchain.

With the changes in this PR

Total Static RAM memory (data + bss): 206034(+0) bytes
Total Flash memory (text + data): 49223(+0) bytes

Without the changes in this PR

Total Static RAM memory (data + bss): 206034(+0) bytes
Total Flash memory (text + data): 49243(+20) bytes

[kjbracey]

Not sure I'd file this for 5.14.1 - there's a small chance of a binary compatibility glitch due to the eliminated out-of-line methods, and it's not really a bug fix.

But if you're keen to get the 5.14 branch warning-free, it's fine.

[0xc0170]

CI started

[mbed-ci]

Test run: FAILED

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

Failed test jobs:

• jenkins-ci/mbed-os-ci_exporter

[0xc0170]

exporters restarted