cppcheck Static Code analysis: "Medium" risk for not initializing _lastSend

[Schallbert]

Hi @Makuna, I'm using your lightweight library in one of my projects and just for fun ran a static code checker tool.

According to that, there is 1 medium risk item for not initializing _lastSend upon construction, fix would be easy:
Lines 396/397,

    uint32_t _lastSend{0};
    uint16_t _lastSendSpace{0};

If I had the rights on the repo, I'd create a branch and a PR myself.

There is another issue from that check that's more... ehm... individual style I believe. It is because the template class has a constructor that is not explicit. Well, that's up to your personal taste - just for completeness' sake.

[Makuna]

@Schallbert Normal process for contributing follows this path:

1. Clone to repo to your Github account
2. Clone from your account to your computer
3. make changes and create pull to your account clone
4. create pull from your github to this master and start the review process

BUT, There are two issues you bring up; one has a reason not to fix the other is a valid one.

A) initialize _lastSend in the constructor:
This is intentionally not done. It is initialized to a workable number in begin, and any number before this point is as valid as any other so having it init to zero is not helpful; BUT it costs 12 bytes of program space to do so. If this was targeted at different platforms I would agree it JUST should be done. But since I target the smallest of uC (like an ATTINY) then 12 bytes of code is significant when it would not cause any ill effects.

B) There is no blocking of a default constructor by declaring a private one:
This is a 100% valid concern; but Arduino will create a compile error if you do already. What platform are you using to build with?

[Schallbert]

Hi and thanks for the quick answer.
for A) I totally agree that nothing can go wrong with the current solution as begin() is timely coupled and will have to happen inevitably before using _lastSend variable (what the code checker cannot know). I don't understand the 12 bytes though, why isn't it just 4? And how about the alternative to brace-initialize like this: uint32_t _lastSend{}; Will this also cost program memory?

for B) I think I created a misunderstanding. I mean the static code checker would list as a weakness on a verbose debug build that the DFMiniMp3 constructor is not strictly typesafe because it would allow implicit casts (of its argument). I fixed this by simply declaring line 87 explicit DFMiniMp3(T_SERIAL_METHOD& serial) :.
I am using framework-arduino-avr 5.0.0 with toolchain-atmelavr 5.4.0 . I don't get compiler warnings/errors.

[Schallbert]

@Makuna BTW how can I learn about what costs how much program memory and at what point RAM usage forecast goes up (as a compile output)? I have a feeling that dynamic allocation cannot show up there at all. I'm struggling on 2k/4k flash systems that cannot even take the debug build because it is non-optimized and thus much bigger than the release build.

[Makuna]

At the end of compile, you get a summary like:

Sketch uses 4060 bytes (1%) of program storage space. Maximum is 253952 bytes.
Global variables use 504 bytes (6%) of dynamic memory, leaving 7688 bytes for local variables. Maximum is 8192 bytes.

The above compared to a defaule initialization using {}

Sketch uses 4072 bytes (1%) of program storage space. Maximum is 253952 bytes.
Global variables use 504 bytes (6%) of dynamic memory, leaving 7688 bytes for local variables. Maximum is 8192 bytes.

Note, this is AVR (mega) compile. If I used Esp8266 there is no difference between uninitialized versus initialized. I suspect a compiler difference.

Correct, dynamic allocations can only be tested while running, with calls to system methods that provide info for free heap and free stack and can be platform dependent.

[Schallbert]

OK OK, so you're not actually calculating that yourself ^^ that's helpful, yes. When I compare my compilation with and without initialization of the value, I even get 16Bytes more. Well, nevermind then, keep A) like it is.
How about B) then?

[Makuna]

adding the explicit makes sense, go ahead and make a pull; check the compile times ;-)
(compile times should not be effected)

[Schallbert]

Compile time ABA test shows: Jitter. No effect as postulated. PR created. Closed.

[Makuna]

Usually wait until your changes are merged in to close the issue associated with it. But that is done now.