Ticket #3598 (closed enhancement: fixed)
[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
Change History
Changed 9 years ago by and
- Attachment mc-3598-0000-widget-common-introduce-CWIDGET-helper.patch added
Changed 9 years ago by and
- Attachment mc-3598-0001-widget-common.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0002-filemanager-panel.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0003-filemanager-boxes.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0005-search-lib.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0006-search-regex.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0007-skin-ini-file.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0008-tty-key.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0009-tty-tty-ncurses.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0010-vfs-path.c-cleanup-some-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0011-widget-dialog.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0012-widget-input.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
Changed 9 years ago by and
- Attachment mc-3598-0014-widget-quick.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0015-utilunix.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0016-filemanager-file.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0017-filemanager-find.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0019-viewer-dialogs.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0020-viewer-search.c-cleanup-some-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0021-editor-choosesyntax.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0022-editor-editcmd.c-cleanup-some-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0023-editor-edit_dialogs.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0024-editor-editdraw.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0025-editor-syntax.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0026-diffviewer-search.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0027-diffviewer-ydiff.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0028-clipboard.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0029-widget-buttonbar.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0030-filemanager-layout.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0032-execute.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
- Attachment mc-3598-0033-subshell-common.c-cleanup-Wcast-qual-warning.patch added
comment:1 Changed 9 years ago by andrew_b
Ok, no try to find and fix memory allocations introduced in these patches.
comment:2 Changed 9 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 9 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 9 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 9 years ago by and
ups, questionable strutil* patches with g_strdup() parts updated
Changed 9 years ago by and
- Attachment mc-3598-0034-strutil-strutil8bit.c-cleanup-Wcast-qual-warning.patch added
Changed 9 years ago by and
Changed 9 years ago by and
comment:6 in reply to: ↑ 5 Changed 9 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 9 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.
comment:8 Changed 9 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 9 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 9 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 9 years ago by andrew_b
- Votes for changeset set to andrew_b
- Branch state changed from on review to approved
comment:12 Changed 9 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