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

Fix cppcheck warnings #114

Merged
merged 3 commits into from Mar 9, 2018

Conversation

Projects
None yet
2 participants
@poire-z
Copy link
Contributor

poire-z commented Mar 7, 2018

Attempt at fixing warnings from #106. Just to see what cppcheck says.
Don't merge yet: it compiles, it seems to work, and my C skills warning still applies :) and there is at least one which I doubt:

This one has 2 * more at start than in the sizeof()

-            rows = (_Ty**) realloc( rows, sizeof(_Ty)*nrows );
+            rows = cr_realloc( rows, nrows );

All the others have one * more at start than in the sizeof()

-            _list = (T**)realloc( _list, size * sizeof( T* ));
+            _list = cr_realloc( _list, size );

Added a inline to this cr_realloc , so I don't have to worry about performance impacts (libcrengine grows by 1 KB only, so it's negligeable).

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Mar 7, 2018

Just to see what cppcheck says.

It's not exactly hard to run it on a file or two. :-P

(Getting a commit range from Travis and running it only on changed files to prevent regressions without always wasting a lot of time is a little more difficult.)

poire-z added some commits Mar 7, 2018

Fix 'Null pointer dereference'
I guess it was because there is a 2nd remove() with the same name
but that accepts a pointer. So remove(0) was ambiguous.
Fix 'Reference to temporary returned'
May be it's cppcheck getting confused, let's see how it does
if we use the other constructor (which should do the same thing if NULL).
@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Mar 7, 2018

Isn't there some cppcheck equivalent to -- luacheck: ignore ?
cppcheck is happy now, but changing things and adding uncertainty just to please it is a bit bothering to me :|

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Mar 7, 2018

Should work like:

// cppcheck-suppress "suppressed_error_id"

Which one are you referring to?

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Mar 7, 2018

The rows = (_Ty**) realloc( rows, sizeof(_Ty)*nrows ); I mention in the first post.
The return LVRef(); => return LVRef(NULL); which is probably ok, but looks less natural.

But again, I'm just timid changing things that work with stuff I do not master (and that I can't explicitely check individually).
Anyway, I'd rather have it reviewed by a more seasoned C/C++ programmer. @frankyifei : are you one ? :)

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Mar 7, 2018

Looks to me like some kind of multidimensional array, not a mistake, and therefore definitely not something to change:

/// returns pointer array
T ** get() { return _list; }

But I don't think cppcheck is wrong. I do, however, doubt whether cr_realloc as written is up to the job.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Mar 7, 2018

So, may be this change actually fix something:

-            rows = (_Ty**) realloc( rows, sizeof(_Ty)*nrows );
+            rows = cr_realloc( rows, nrows );

Originally, if rows are a table of pointers to cols, themselves pointing to things (_Ty), it was allocating memory for nrows * the size of that thing, which is probably a lot more (in src/lvrend.cpp, _Ty is a class CCRTableCell, holding a few int, short, and pointers) than nrows * the size of a pointer.
With cr_realloc(), it may do the right thing, with allocating the nrows * the size of what it holds: pointers.

@poire-z

This comment has been minimized.

Copy link
Contributor Author

poire-z commented Mar 8, 2018

Should we dare merging this ? (people can still have a look later, we'll react if there's better solutions or if we notice strange things)

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Mar 8, 2018

I don't see anything obviously wrong, although I'm not quite sure about the double pointer stuff. If your testing shows nothing obviously wrong and the unit tests pass I suppose at least it wouldn't introduce any horrible regressions. :-P

@poire-z poire-z merged commit 85742d0 into koreader:master Mar 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@poire-z poire-z deleted the poire-z:cppcheck_shutup branch Mar 9, 2018

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.