Various cleanups #1418
Various cleanups #1418
Conversation
This is *not* an issue, as it's not really a bounds check but a mere limit, and the array is NULL-terminated. However, it looks odd and cppcheck (unsurprisingly) doesn't understand the details well enough and reports this as a misordered bounds check. > mate-panel/panel-run-dialog.c:166:12: style: Array index 'i' is used before limits check. [arrayIndexThenCheck] > items[i] && i < history_max_size; > ^
cppcheck tells us that we perform NULL checking inconsistently, and
reports a potential NULL dereference. Here however the string cannot
be NULL, so just drop the unnecessary check.
> mate-panel/panel-run-dialog.c:1663:10: warning: Either the condition '!start' is redundant or there is possible null pointer dereference: start. [nullPointerRedundantCheck]
> while (*start != '\0' && g_ascii_isspace (*start))
> ^
> mate-panel/panel-run-dialog.c:1679:6: note: Assuming that condition '!start' is not redundant
> if (!start || !start [0]) {
> ^
> mate-panel/panel-run-dialog.c:1663:10: note: Null pointer dereference
> while (*start != '\0' && g_ascii_isspace (*start))
> ^
There is no need to return -1 or 1 specifically, anything on the right side of 0 is OK, so simplify using the usual `a - b` implementation for such sort functions. Note that the type is OK here, as the `struct tm` are ints, the same as the function's return value.
- Build the list in reverse order, then reverse the result. This is useful because GS?List are list nodes, not containers of nodes, and thus don't contain a pointer to the list's end, meaning to append one has to walk the entire list to find the end each time. To avoid this we use the common idiom of prepending to the list (which is cheap, as it's adding a node before the given one), and then reversing the resulting list to get back the original order. - Avoid unnecessary memory copy by stealing the GStrv's members. We get the array as a copy, so we can simply steal the members and free the container array only, saving a copy for each member.
> mate-panel/panel-action-button.c:516:26: style: struct member 'PanelAction::type' is never used. [unusedStructMember] > PanelActionButtonType type; > ^
Mostly found by cppcheck.
No need to get the default icon theme again anyway.
This is not strictly needed as there's already an assertion (assuming it's not compiled out), but returning a value help analyzers get this right.
It's probably not necessary to perform the NULL check at all as the only code paths that could make `list` NULL are already pretty dramatically broken, but as we have a NULL check move everything relevant inside it.
`info` cannot be NULL, and if it was it'd already have crashed above. > mate-panel/panel.c:893:34: warning: Either the condition 'info!=((void*)0)' is redundant or there is possible null pointer dereference: info. [nullPointerRedundantCheck] > parent = gtk_widget_get_parent (info->widget); > ^ > mate-panel/panel.c:895:11: note: Assuming that condition 'info!=((void*)0)' is not redundant > if (info != NULL && > ^ > mate-panel/panel.c:893:34: note: Null pointer dereference > parent = gtk_widget_get_parent (info->widget); > ^
The computed values are only used in one branch, so compute them there.
There was a problem hiding this comment.
No build issues, and I tested panel lockdown, disabling force-quit, changing panel backgrounds (both image and color), the panel run dialog, adding/removing clock/calendar locations, and tested wayland as well. The multimonitor behavior was same as before: panel on primary monitor in x11, and on wayland on whichever monitor a terminal it is called from is on or the leftmost monitor if called by the session.
This is but a quick test, but it looks good on my end and addresses lots of "papercut" bugs in the code itself. Lots to test, but would be more if the same work was split into a dozen PR's that have to be pulled separately.
|
Beside from the new warning it seems that the panel runs fine in quick test. |
|
What is the status of this? due to the number of file changes we may have to rewrite either this or other PR's to deal with merge conflicts, so I would prefer to have this finished and out of the way. The cleanups improve the quality of the code, and the cppcheck warning is but advice on how code could be further improved, not a build warning. Holding on merging this as dealing with that is an unanswered question at this time |
No need to have pointers to mutables when not needed. This does not cover everything at all, and is limited to one file for the moment.
|
@lukefromdc IMO it's ready. I addressed @raveit65's comment although it's not really new from this PR, but my additional commit just marks a bunch of pointers const, so either the compiler will emit new warnings, or it's almost certainly free of behavioral changes. |
|
This works fine, save for not containing the pager hack which for obvious reasons in in a different PR |
|
Accidental closure on comment |
All this was mostly found by cppcheck while I tried to improve its output in #1415. Not everything it reports is taken care of here, but a lot is.