Ticket #3598 (closed enhancement: fixed)

Opened 8 years ago

Last modified 8 years ago

[patch] -Wcast-qual cleanup

Reported by: and Owned by: andrew_b
Priority: minor Milestone: 4.8.17
Component: mc-core Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

most (but not all) -Wcast-qual cleanup.

By -Wcast-qual flag compiler warns when const declaration is violated.

Attachments

mc-3598-0000-widget-common-introduce-CWIDGET-helper.patch (1.1 KB) - added by and 8 years ago.
mc-3598-0001-widget-common.c-cleanup-Wcast-qual-warning.patch (1.2 KB) - added by and 8 years ago.
mc-3598-0002-filemanager-panel.c-cleanup-Wcast-qual-warning.patch (8.6 KB) - added by and 8 years ago.
mc-3598-0003-filemanager-boxes.c-cleanup-Wcast-qual-warning.patch (3.9 KB) - added by and 8 years ago.
mc-3598-0005-search-lib.c-cleanup-Wcast-qual-warning.patch (1.2 KB) - added by and 8 years ago.
mc-3598-0006-search-regex.c-cleanup-Wcast-qual-warning.patch (1.8 KB) - added by and 8 years ago.
mc-3598-0007-skin-ini-file.c-cleanup-Wcast-qual-warning.patch (2.2 KB) - added by and 8 years ago.
mc-3598-0008-tty-key.c-cleanup-Wcast-qual-warning.patch (4.4 KB) - added by and 8 years ago.
mc-3598-0009-tty-tty-ncurses.c-cleanup-Wcast-qual-warning.patch (919 bytes) - added by and 8 years ago.
mc-3598-0010-vfs-path.c-cleanup-some-Wcast-qual-warning.patch (2.0 KB) - added by and 8 years ago.
mc-3598-0011-widget-dialog.c-cleanup-Wcast-qual-warning.patch (2.0 KB) - added by and 8 years ago.
mc-3598-0012-widget-input.c-cleanup-Wcast-qual-warning.patch (1.2 KB) - added by and 8 years ago.
mc-3598-0013-widget-input_complete.c-cleanup-Wcast-qual-warning.patch (2.3 KB) - added by and 8 years ago.
mc-3598-0014-widget-quick.c-cleanup-Wcast-qual-warning.patch (1.3 KB) - added by and 8 years ago.
mc-3598-0015-utilunix.c-cleanup-Wcast-qual-warning.patch (2.3 KB) - added by and 8 years ago.
mc-3598-0016-filemanager-file.c-cleanup-Wcast-qual-warning.patch (2.7 KB) - added by and 8 years ago.
mc-3598-0017-filemanager-find.c-cleanup-Wcast-qual-warning.patch (4.8 KB) - added by and 8 years ago.
mc-3598-0018-extfs.c-cleanup-Wcast-qual-warning.patch (952 bytes) - added by and 8 years ago.
mc-3598-0019-viewer-dialogs.c-cleanup-Wcast-qual-warning.patch (1.5 KB) - added by and 8 years ago.
mc-3598-0020-viewer-search.c-cleanup-some-Wcast-qual-warning.patch (1.1 KB) - added by and 8 years ago.
mc-3598-0021-editor-choosesyntax.c-cleanup-Wcast-qual-warning.patch (1.1 KB) - added by and 8 years ago.
mc-3598-0022-editor-editcmd.c-cleanup-some-Wcast-qual-warning.patch (3.8 KB) - added by and 8 years ago.
mc-3598-0023-editor-edit_dialogs.c-cleanup-Wcast-qual-warning.patch (3.0 KB) - added by and 8 years ago.
mc-3598-0024-editor-editdraw.c-cleanup-Wcast-qual-warning.patch (1.1 KB) - added by and 8 years ago.
mc-3598-0025-editor-syntax.c-cleanup-Wcast-qual-warning.patch (3.4 KB) - added by and 8 years ago.
mc-3598-0026-diffviewer-search.c-cleanup-Wcast-qual-warning.patch (1.3 KB) - added by and 8 years ago.
mc-3598-0027-diffviewer-ydiff.c-cleanup-Wcast-qual-warning.patch (4.9 KB) - added by and 8 years ago.
mc-3598-0028-clipboard.c-cleanup-Wcast-qual-warning.patch (912 bytes) - added by and 8 years ago.
mc-3598-0029-widget-buttonbar.c-cleanup-Wcast-qual-warning.patch (1.9 KB) - added by and 8 years ago.
mc-3598-0030-filemanager-layout.c-cleanup-Wcast-qual-warning.patch (1.3 KB) - added by and 8 years ago.
mc-3598-0031-setup.c-cleanup-Wcast-qual-warning.patch (1.3 KB) - added by and 8 years ago.
mc-3598-0032-execute.c-cleanup-Wcast-qual-warning.patch (1.1 KB) - added by and 8 years ago.
mc-3598-0033-subshell-common.c-cleanup-Wcast-qual-warning.patch (1.0 KB) - added by and 8 years ago.
mc-3598-0004-paths.c-cleanup-Wcast-qual-warning.patch (2.8 KB) - added by and 8 years ago.
mc-3598-0034-strutil-strutil8bit.c-cleanup-Wcast-qual-warning.patch (5.4 KB) - added by and 8 years ago.
mc-3598-0035-strutil-strutilascii.c-cleanup-Wcast-qual-warning.patch (4.6 KB) - added by and 8 years ago.
mc-3598-0036-strutil-strutilutf8.c-cleanup-some-Wcast-qual-warnings.patch (2.9 KB) - added by and 8 years ago.

Change History

comment:1 Changed 8 years ago by andrew_b

Ok, no try to find and fix memory allocations introduced in these patches.

Upd.
s/no/now
a/allocations/memleaks
Sorry for mistakes.

Version 2, edited 8 years ago by andrew_b (previous) (next) (diff)

comment:2 Changed 8 years ago by zaytsev

I think it would be great to get the const-correctness fixed, but one needs to inspect what the callers & callees are doing very carefully, sometimes adapting the return types and sometimes the interface, but simply replacing const_casts with g_strdup is not a solution.

I've just looked at the few first and few last patches, and while many look okay and safe, the strutil* changes definitively feel wrong, just as the skin_name_default stuff, etc.

@and, re. mc-3598-0033-subshell-common.c-cleanup-Wcast-qual-warning.patch:

See an example here on how to use this function correctly without causing memory leaks:

http://pubs.opengroup.org/onlinepubs/9699919799/functions/putenv.html

In the particular case of putenv(), there is an alternative called setenv(), but probably we don't want to switch to it (I'm not sure of the implementation status).

Same approach can be used at all places where you replace constants with heap allocated strings, but for some reason you can't add const qualifier to the function interface.

However, this should be an exception rather than the rule.

comment:3 follow-up: ↓ 4 Changed 8 years ago by and

I have created patches per file, so you can pick/ACK or NAK per file easily.
Remaining patches are a basis for discussion and yes I not free of errors.

Lets take this for example from
mc-3598-0033-subshell-common.c-cleanup-Wcast-qual-warning.patch:

-    return (case_sen) ? (char *) text : str_8bit_strdown (text);
+    return (case_sen) ? g_strdup (text) : str_8bit_strdown (text);

str_8bit_strdown() will already return a s_strdup'ed value,
so by should be g_strdup(text) false?

By the way I found a compile warning introduction when dealing without XDG support, patch updated.

comment:4 in reply to: ↑ 3 Changed 8 years ago by andrew_b

Replying to and:

-    return (case_sen) ? (char *) text : str_8bit_strdown (text);
+    return (case_sen) ? g_strdup (text) : str_8bit_strdown (text);

str_8bit_strdown() will already return a s_strdup'ed value,
so by should be g_strdup(text) false?

Because

    if (!case_sen)
        g_free (key);

comment:5 follow-up: ↓ 6 Changed 8 years ago by and

ups, questionable strutil* patches with g_strdup() parts updated

comment:6 in reply to: ↑ 5 Changed 8 years ago by andrew_b

Replying to and:

ups, questionable strutil* patches with g_strdup() parts updated

I don't like string duplication. If we can do without that we should.

comment:7 Changed 8 years ago by andrew_b

  • Owner set to andrew_b
  • Status changed from new to accepted
  • Milestone changed from Future Releases to 4.8.17

Branch: 3598_wcast_qual.
Initial changeset:f2ea9630614d20212eaaaa7aaec388025d06f34b

mc-3598-0000-widget-common-introduce-CWIDGET-helper.patch​: applied with rename CWIDGET -> CONST_WIDGET.
mc-3598-0001-widget-common.c-cleanup-Wcast-qual-warning.patch​: applied.
mc-3598-0002-filemanager-panel.c-cleanup-Wcast-qual-warning.patch​: applied.
mc-3598-0003-filemanager-boxes.c-cleanup-Wcast-qual-warning.patch​: not applied yet.
mc-3598-0004-paths.c-cleanup-Wcast-qual-warning.patch: applied.
mc-3598-0005-search-lib.c-cleanup-Wcast-qual-warning.patch​: applied.
mc-3598-0006-search-regex.c-cleanup-Wcast-qual-warning.patch​: applied.
mc-3598-0007-skin-ini-file.c-cleanup-Wcast-qual-warning.patch​: applied.
mc-3598-0008-tty-key.c-cleanup-Wcast-qual-warning.patch: applied.
mc-3598-0009-tty-tty-ncurses.c-cleanup-Wcast-qual-warning.patch: applied.
mc-3598-0010-vfs-path.c-cleanup-some-Wcast-qual-warning.patch: applied.

Other patches will be reviewed later.

Last edited 8 years ago by andrew_b (previous) (diff)

comment:8 Changed 8 years ago by andrew_b

mc-3598-0011-widget-dialog.c-cleanup-Wcast-qual-warning.patch: applied.
mc-3598-0012-widget-input.c-cleanup-Wcast-qual-warning.patch: not applied because of different fix.
mc-3598-0013-widget-input_complete.c-cleanup-Wcast-qual-warning.patch: applied.
mc-3598-0014-widget-quick.c-cleanup-Wcast-qual-warning.patch: not applied yet.
mc-3598-0015-utilunix.c-cleanup-Wcast-qual-warning.patch: skipped because of extra string duplication.
mc-3598-0016-filemanager-file.c-cleanup-Wcast-qual-warning.patch: applied with some modification (like of patch 0012).
mc-3598-0017-filemanager-find.c-cleanup-Wcast-qual-warning.patch: applied partially because of extra string duplication. Another fix is required.
mc-3598-0018-extfs.c-cleanup-Wcast-qual-warning.patch: applied.
mc-3598-0019-viewer-dialogs.c-cleanup-Wcast-qual-warning.patch: not applied yet.
mc-3598-0020-viewer-search.c-cleanup-some-Wcast-qual-warning.patch: applied.
mc-3598-0021-editor-choosesyntax.c-cleanup-Wcast-qual-warning.patch: applied.

Other patches will be reviewed later.

comment:9 Changed 8 years ago by andrew_b

mc-3598-0022-editor-editcmd.c-cleanup-some-Wcast-qual-warning.patch: applied.
mc-3598-0023-editor-edit_dialogs.c-cleanup-Wcast-qual-warning.patch: applied partially.
mc-3598-0024-editor-editdraw.c-cleanup-Wcast-qual-warning.patch: applied.
mc-3598-0025-editor-syntax.c-cleanup-Wcast-qual-warning.patch: applied.
mc-3598-0026-diffviewer-search.c-cleanup-Wcast-qual-warning.patch: not applied yet.
mc-3598-0027-diffviewer-ydiff.c-cleanup-Wcast-qual-warning.patch: applied partially with another fix.
mc-3598-0028-clipboard.c-cleanup-Wcast-qual-warning.patch: applied.
mc-3598-0029-widget-buttonbar.c-cleanup-Wcast-qual-warning.patch: applied.
mc-3598-0030-filemanager-layout.c-cleanup-Wcast-qual-warning.patch: skipped because of extra string duplication.
mc-3598-0031-setup.c-cleanup-Wcast-qual-warning.patch: applied.
mc-3598-0032-execute.c-cleanup-Wcast-qual-warning.patch: skipped because of extra string duplication.
mc-3598-0033-subshell-common.c-cleanup-Wcast-qual-warning.patch: skipped because of unneeded string duplication.
mc-3598-0034-strutil-strutil8bit.c-cleanup-Wcast-qual-warning.patch: skipped because of unneeded string duplication.
mc-3598-0035-strutil-strutilascii.c-cleanup-Wcast-qual-warning.patch: skipped because of unneeded string duplication.
mc-3598-0036-strutil-strutilutf8.c-cleanup-some-Wcast-qual-warnings.patch: skipped because of unneeded string duplication.

comment:10 Changed 8 years ago by andrew_b

  • Branch state changed from no branch to on review

Changesets are reodered.

Branch: 3598_wcast_qual.
Initial changeset:767287c9d2e15e25bb7e5c64fff4e9322d6b95ba

I thing this is enough for first iteration. Please review.

comment:11 Changed 8 years ago by andrew_b

  • Votes for changeset set to andrew_b
  • Branch state changed from on review to approved

comment:12 Changed 8 years ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

Merged to master: [9d0f6e584ef86daeb421a5d63898ee2effe8f5d8].

git log --pretty=oneline 51d0783..9d0f6e5

comment:13 Changed 8 years ago by andrew_b

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.