lancos / ponyprog

Serial device programmer
GNU General Public License v2.0
73 stars 13 forks source link

Fixed some issues found by PVS-Studio static code analyser #4

Closed mlytvyn80 closed 6 years ago

mlytvyn80 commented 6 years ago
eduard-x commented 6 years ago

Hi! Thanks for report! I think, the cases like this


unsigned` char *localbuf = new unsigned char[readpage_size];

 if (localbuf == 0)
 {
    return OUTOFMEMORY;
 }

we can leave it at that because of return value from new/malloc. Of cause, if we have no more memory, is fatal. But static analyses tools like cppcheck or gcc option for memory sanitizer print out the warning messages.

mlytvyn80 commented 6 years ago

Hi,

actually according to the C++ standard operator new does not return null pointer. It triggers an exception. So, you code does not make sense.

Regards, Mykhailo

Do., 19. Okt. 2017 о 10:39 Eduard Kalinowski notifications@github.com пише:

Hi! Thanks for report! I think, the cases like this `unsigned char *localbuf = new unsigned char[readpage_size];

  • if (localbuf == 0)

  • {

  • return OUTOFMEMORY;

  • }`

we can leave it at that because of return value from new/malloc. Of cause, if we have no more memory, is fatal. But static analyses tools like cppcheck or gcc option for memory sanitizer print out the warning messages.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/lancos/ponyprog/pull/4#issuecomment-337839703, or mute the thread https://github.com/notifications/unsubscribe-auth/AKPmZdhOEqrjJ7yFPq2393V6g6vg28-Aks5stwrOgaJpZM4P8A-2 .

-- With best regards, Mykhailo

lancos commented 6 years ago

I think it's true, in standard C++11 the new doesn't return 0, but instead raise an exception. http://www.cplusplus.com/reference/new/operator%20new/

Now we have two options: 1) remove the check like suggested by Mykhailo, the exception raised should be a remote event after all. 2) keep the check and pass (std::nothrow) to new operator, this option needs to be tested.

What do you think?

I will review other changes more carefully tomorrow.

eduard-x commented 6 years ago

I think, better is the solution from Mykhailo :+1: