search

fenugrec / freediag

OBD2 scantool
GNU General Public License v3.0
338 stars 75 forks source link

Remove unsignedLessThanZero warnings and suppression. #71

Closed cfehse closed 3 years ago

cfehse commented 3 years ago

Scope

This is a quick follow-up to PR #70 to removed the unsignedLessThanZero warning and suppressions from the source tree/build system.

What has changed?

Source Files:

scantool/CMakeLists.txt

cppcheck\win32.cppcheck cppcheck\linux.cppcheck

various source files

Tested Environments:

All changes where tested using these environments:

Cppcheck:

Compiler/Build Environment:

Perhaps you like the changes and accept this merge request.

Thanks!

@fenugrec

fenugrec commented 3 years ago

You're too fast ! I was going to look at those : )

But for those len == 0 checks, I'm considering instead using assert() as I mentioned in https://github.com/fenugrec/freediag/issues/68 . There is no reason to ever call those functions with length 0, only if there's a bug at a higher level. But I'm not sure yet if asserting is appropriate. I'm generally trying to use assert() more often as it gives information that is impossible to ignore.

No need to change your PR for now, I need to think a bit more on this (happy to hear your opinion, too)

cfehse commented 3 years ago

In C++ the code should throw an exception (regardless of debug or release). Another point is that the place where the check is made is not the root cause of the problem. So the error should be propagated to a higher level or central spot in the application (what the diag_iseterr() function does?). Somehow I don't like that asserts are replaced by "nothing" in release mode.

Perhaps something like this would be feasible:

assert(len);
if (len == 0) {
    return diag_iseterr(DIAG_ERR_BADLEN);
}

I find C difficult in this regard.

fenugrec commented 3 years ago

Another point is that the place where the check is made is not the root cause of the problem.

Correct - the idea is to get a stack trace from i.e. gdb.

Somehow I don't like that asserts are replaced by "nothing" in release mode.

Ah, but my releases always have asserts enabled ! In other projects, I use a custom assert macro that is always enabled (regardless of NDEBUG).

But the reality is, few people are using freediag, so all this is mostly academic...

cfehse commented 3 years ago

Ah, but my releases always have asserts enabled !

Then by all means add assert there.

Correct - the idea is to get a stack trace from i.e. gdb.

May I ask what your personal development environment looks like? (The last time I got a stack trace from a core dump with dbx was the better part of 25 years ago)

But the reality is, few people are using freediag, so all this is mostly academic...

Yeah it's academic - so what? This 50 source files have so much research and passion in them - why not keeping them alive. I find this fun and for me it's a way to stay a little "current and qualified". gg

Can all the changed checks in this PR be replaced by asserts? If so I will do that real quit.

cfehse commented 3 years ago

I added some information about the build process with VS/VSCode to the doc/build_system.txt file. I leave this change in the separate commit.

fenugrec commented 3 years ago

May I ask what your personal development environment looks like? (The last time I got a stack trace from a core dump with dbx was the better part of 25 years ago)

Since a few years ago I now run Linux on all my machines, with XP + win7 VMs for the occasional test or exclusive software. I compile with native gcc, or cross-compile with mingw-w64 when building for windows (now rare, and only for a handful of projects). my VMs have gdb installed simply because it's a familiar tool that can process debug builds. In case of a reported segfault or crash, ideally I try to reproduce locally. So far this has usually been possible, and it lets me hook up with gdb and view the stack trace. I don't remember anyone ever sending a core dump, but it "should be simple" to load it post-crash in gdb and analyze... in theory.

This 50 source files have so much research and passion in them - why not keeping them alive.

Absolutely. But I should be spending time on more important problems instead of meditating on "to assert or not" P )

Can all the changed checks in this PR be replaced by asserts? If so I will do that real quit.

Thanks, won't be necessary at the moment. I will try to merge this today, and open an issue for the general question of asserts . I think it deserves its own discussion and separate commits that address the whole codebase, not just the special case of len==0 in the L0 layer. This PR was initially for the <= 0 warning and suppression, and is perfectly adequate for that.

cfehse commented 3 years ago

I don't remember anyone ever sending a core dump, but it "should be simple" to load it post-crash in gdb and analyze... in theory.

Our C guys did that back then...

I will try to merge this today

Okay. Next I will take a look to create a github action workflow to make something happen there.

Absolutely. But I should be spending time on more important problems instead of meditating on "to assert or not" P )

I take a look at the issues with the "help wanted" label.

fenugrec commented 3 years ago

Our C guys did that back then...

Ah ! How would you (or they) do it today ?

cfehse commented 3 years ago

Ah ! How would you (or they) do it today ?

If this project were started let's say around 2010 or so it would likely be implemented in C++ and maybe use something like this http://boostorg.github.io/stacktrace/index.html for this purpose. Back in the day we were working on 80x24 ASCII terminals using vi/make/dbx - now one has better options. gg