git: remove unneeded casts
PATCH 1/1 git: remove unneeded casts
From: Seija Kijin <[email protected]>
Many of these casts remain,
even though the target variable is the type it is being casted to. We can safely remove said casts.
---
git: remove unneeded casts
Many of these casts remain, even though the target variable is the type it is being casted to. We can safely remove said casts.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1396%2FAtariDreams%2Ffix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1396/AtariDreams/fix-v1
Pull-Request: https://github.com/git/git/pull/1396
builtin/credential-store.c | 2 +- diff.c | 8 ++++---- http.c | 4 ++-- imap-send.c | 5 +++-- merge-ort.c | 2 +- oidmap.h | 2 +- pack-revindex.c | 3 ++- read-cache.c | 2 +- ref-filter.c | 2 +- strbuf.c | 2 +- t/helper/test-parse-options.c | 2 +- t/helper/test-path-utils.c | 8 ++++++-- t/helper/test-run-command.c | 10 +++++----- t/helper/test-subprocess.c | 2 +- thread-utils.c | 2 +- xdiff/xprepare.c | 2 +- xdiff/xutils.c | 2 +- 17 files changed, 33 insertions(+), 27 deletions(-)
builtin/credential-store.c +1 -1
@@ -165,7 +165,7 @@ int cmd_credential_store(int argc, const char **argv, const char *prefix)umask (077 ); - argc =parse_options (argc, (const char **)argv, prefix, options, usage,0 ); + argc =parse_options (argc, argv, prefix, options, usage,0 );if (argc !=1 )usage_with_options (usage, options); op = argv[0 ];
diff.c +4 -4
@@ -1776,8 +1776,8 @@ static void emit_rewrite_diff(const char *name_a, ecbdata.opt = o;if (ecbdata.ws_rule &WS_BLANK_AT_EOF ) {mmfile_t mf1, mf2; - mf1.ptr = (char *)data_one; - mf2.ptr = (char *)data_two; + mf1.ptr = data_one; + mf2.ptr = data_two; mf1.size = size_one; mf2.size = size_two;check_blank_at_eof (&mf1, &mf2, &ecbdata);
@@ -1809,9 +1809,9 @@ static void emit_rewrite_diff(const char *name_a,if (lc_b)emit_rewrite_lines (&ecbdata,'+' , data_two, size_two);if (textconv_one) -free ((char *)data_one); +free (data_one);if (textconv_two) -free ((char *)data_two); +free (data_two); }struct diff_words_buffer {
http.c +2 -2
@@ -2319,8 +2319,8 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb, } do { - ssize_t retval = xwrite(freq->localfile, - (char *) ptr + posn, size - posn); + ssize_t retval = + xwrite(freq->localfile, ptr + posn, size - posn); if (retval < 0) return posn / eltsize; posn += retval;
imap-send.c +3 -2
@@ -779,7 +779,7 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)if (cmdp->cb.data) { n =socket_write (&imap->buf.sock, cmdp->cb.data, cmdp->cb.dlen);FREE_AND_NULL (cmdp->cb.data); -if (n != (int )cmdp->cb.dlen) +if (n != cmdp->cb.dlen)return RESP_BAD ; }else if (cmdp->cb.cont) {if (cmdp->cb.cont (ctx, cmdp, cmd))
@@ -1526,7 +1526,8 @@ int cmd_main(int argc, const char **argv)setup_git_directory_gently (&nongit_ok);git_config (git_imap_config,NULL ); - argc =parse_options (argc, (const char **)argv,"" , imap_send_options, imap_send_usage,0 ); + argc =parse_options (argc, argv,"" , imap_send_options, imap_send_usage, +0 );if (argc)usage_with_options (imap_send_usage, imap_send_options);
merge-ort.c +1 -1
@@ -2574,7 +2574,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,/* Find parent directories missing from opt->priv->paths */ cur_path =mem_pool_strdup (&opt->priv->pool, new_path); -free ((char *)new_path); +free (new_path); new_path = (char *)cur_path;while (1 ) {
oidmap.h +1 -1
@@ -87,7 +87,7 @@ static inline void *oidmap_iter_first(struct oidmap *map, {oidmap_iter_init (map, iter);/* TODO: this API could be reworked to do compile-time type checks */ -return (void *)oidmap_iter_next (iter); +return oidmap_iter_next (iter); }#endif
pack-revindex.c +2 -1
@@ -428,7 +428,8 @@ static int midx_pack_order_cmp(const void *va, const void *vb)const struct midx_pack_key *key = va;struct multi_pack_index *midx = key->midx; -uint32_t versus =pack_pos_to_midx (midx, (uint32_t *)vb - (const uint32_t *)midx->revindex_data); +uint32_t versus = +pack_pos_to_midx (midx, (uint32_t *)vb - midx->revindex_data);uint32_t versus_pack =nth_midxed_pack_int_id (midx, versus);off_t versus_offset;
read-cache.c +1 -1
@@ -3022,7 +3022,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, offset =hashfile_total (f); } -if (ce_write_entry (f, ce, previous_name, (struct ondisk_cache_entry *)&ondisk) <0 ) +if (ce_write_entry (f, ce, previous_name, &ondisk) <0 ) err =-1 ;if (err)
ref-filter.c +1 -1
@@ -396,7 +396,7 @@ static int trailers_atom_parser(struct ref_format *format, struct used_atom *atostrbuf_addf (err,_ ("expected %%(trailers:key=<value>)" )); else strbuf_addf(err, _("unknown %%(trailers) argument: %s"), invalid_arg); -free ((char *)invalid_arg); +free (invalid_arg);return -1 ; } }
strbuf.c +1 -1
@@ -493,7 +493,7 @@ void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags)if (ch <=0x1F || ch >=0x7F || (ch =='/' && (flags &STRBUF_ENCODE_SLASH )) ||strchr (URL_UNSAFE_CHARS , ch)) -strbuf_addf (dst,"%%%02X" , (unsigned char )ch); +strbuf_addf (dst,"%%%02X" , ch);else strbuf_addch (dst, ch); }
t/helper/test-parse-options.c +1 -1
@@ -161,7 +161,7 @@ int cmd__parse_options(int argc, const char **argv)trace2_cmd_name ("_parse_" ); - argc =parse_options (argc, (const char **)argv, prefix, options, usage,0 ); + argc =parse_options (argc, argv, prefix, options, usage,0 );if (length_cb.called) {const char *arg = length_cb.arg;
t/helper/test-path-utils.c +6 -2
@@ -276,13 +276,17 @@ static int protect_ntfs_hfs_benchmark(int argc, const char **argv)for (j =0 ; j < nr; j++)verify_path (names[j], file_mode); end =getnanotime (); -printf ("protect_ntfs = %d, protect_hfs = %d: %lfms\n" , protect_ntfs, protect_hfs, (end-begin) / (double )1e6 ); +printf ("protect_ntfs = %d, protect_hfs = %d: %lfms\n" , + protect_ntfs, protect_hfs, + (end - begin) /1e6 ); cumul += end - begin; cumul2 += (end - begin) * (end - begin); } m[protect_ntfs][protect_hfs] = cumul / (double )repetitions; v[protect_ntfs][protect_hfs] =my_sqrt (cumul2 / (double )repetitions - m[protect_ntfs][protect_hfs] * m[protect_ntfs][protect_hfs]); -printf ("mean: %lfms, stddev: %lfms\n" , m[protect_ntfs][protect_hfs] / (double )1e6 , v[protect_ntfs][protect_hfs] / (double )1e6 ); +printf ("mean: %lfms, stddev: %lfms\n" , + m[protect_ntfs][protect_hfs] /1e6 , + v[protect_ntfs][protect_hfs] /1e6 ); }for (protect_ntfs =0 ; protect_ntfs <2 ; protect_ntfs++)
t/helper/test-run-command.c +5 -5
@@ -312,8 +312,8 @@ static int quote_stress_test(int argc, const char **argv)if (ret) {fprintf (stderr,"Trial #%d failed. Arguments:\n" , i);for (j =0 ; j < arg_count; j++) -fprintf (stderr,"arg #%d: '%s'\n" , - (int )j, args.v[j + arg_offset]); +fprintf (stderr,"arg #%d: '%s'\n" , j, + args.v[j + arg_offset]);strbuf_release (&out);strvec_clear (&args);
@@ -322,7 +322,7 @@ static int quote_stress_test(int argc, const char **argv) }if (i && (i %100 ) ==0 ) -fprintf (stderr,"Trials completed: %d\n" , (int )i); +fprintf (stderr,"Trials completed: %d\n" , i); }strbuf_release (&out);
@@ -418,7 +418,7 @@ int cmd__run_command(int argc, const char **argv) ret =1 ; goto cleanup; } -strvec_pushv (&proc.args, (const char **)argv +2 ); +strvec_pushv (&proc.args, argv +2 );if (!strcmp (argv[1 ],"start-command-ENOENT" )) {if (start_command (&proc) <0 && errno ==ENOENT ) {
@@ -441,7 +441,7 @@ int cmd__run_command(int argc, const char **argv) jobs =atoi (argv[2 ]);strvec_clear (&proc.args); -strvec_pushv (&proc.args, (const char **)argv +3 ); +strvec_pushv (&proc.args, argv +3 );if (!strcmp (argv[1 ],"run-command-parallel" )) { opts.get_next_task = parallel_next;
t/helper/test-subprocess.c +1 -1
@@ -15,6 +15,6 @@ int cmd__subprocess(int argc, const char **argv) argv++; } cp.git_cmd =1 ; -strvec_pushv (&cp.args, (const char **)argv +1 ); +strvec_pushv (&cp.args, argv +1 );return run_command (&cp); }
thread-utils.c +1 -1
@@ -57,7 +57,7 @@ int online_cpus(void)#endif /* defined(HAVE_BSD_SYSCTL) && defined(HW_NCPU) */ #ifdef _SC_NPROCESSORS_ONLN -if ((ncpus = (long )sysconf (_SC_NPROCESSORS_ONLN)) >0 ) +if ((ncpus =sysconf (_SC_NPROCESSORS_ONLN)) >0 )return (int )ncpus;#endif
xdiff/xprepare.c +1 -1
@@ -188,7 +188,7 @@ static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_if (!(crec =xdl_cha_alloc (&xdf->rcha))) goto abort; crec->ptr = prev; - crec->size = (long ) (cur - prev); + crec->size = (cur - prev); crec->ha = hav; recs[nrec++] = crec;if (xdl_classify_record (pass, cf, rhash, hbits, crec) <0 )
xdiff/xutils.c +1 -1
@@ -130,7 +130,7 @@ long xdl_guess_lines(mmfile_t *mf, long sample) {else cur++; } - tsize += (long ) (cur - data); + tsize += (cur - data); }if (nl && tsize)
base-commit: 57e2c6ebbe7108b35ba30184dcbcb6c34c929ad8
Signature · 1 line
gitgitgadget
Quoted 5 lines · Rose via GitGitGadget
From: Seija Kijin <[email protected]>
Many of these casts remain,
even though the target variable is the type it is being casted to. We can safely remove said casts.
Funny line-breaking. Re-wrapping with vim yields:
Many of these casts remain, even though the target variable is the type it is being cast to. We can safely remove said casts.
I also fixed a minor grammatical issue with "casted".
I'm curious how you found all of these. With coccinelle or similar? If there's an analysis tool that can find them, that would make review much simpler if we can trust that it did the leg-work of checking the types.
Either way, I think it's still useful to see how we ended up in this state for each case (so we know that cleaning up the cast is the right thing, and it is not a symptom of some other bug).
builtin/credential-store.c int cmd_credential_store(int argc, const char **argv, const char *prefix) · line 171
Quoted 13 lines
builtin/credential-store.c +1
-1@@ -165,7 +165,7 @@ int cmd_credential_store(int argc, const char **argv, const char *prefix)umask (077 ); - argc =parse_options (argc, (const char **)argv, prefix, options, usage,0 ); + argc =parse_options (argc, argv, prefix, options, usage,0 );if (argc !=1 )usage_with_options (usage, options); op = argv[0 ];
Our argv used to be "char **" due to 84d32bf767 (sparse: Fix mingw_main() argument number/type errors, 2013-04-27), but later regained its const when it became cmd_main() in 3f2e2297b9 (add an extra level of indirection to main(), 2016-07-01). Makes sense.
Quoted 16 lines
diff.c +2
-2@@ -1776,8 +1776,8 @@ static void emit_rewrite_diff(const char *name_a, ecbdata.opt = o;if (ecbdata.ws_rule &WS_BLANK_AT_EOF ) {mmfile_t mf1, mf2; - mf1.ptr = (char *)data_one; - mf2.ptr = (char *)data_two; + mf1.ptr = data_one; + mf2.ptr = data_two; mf1.size = size_one; mf2.size = size_two;check_blank_at_eof (&mf1, &mf2, &ecbdata);
diff.c static void emit_rewrite_diff(const char *name_a, · line 1815
Quoted 9 lines
diff.c +2
-2@@ -1809,9 +1809,9 @@ static void emit_rewrite_diff(const char *name_a,if (lc_b)emit_rewrite_lines (&ecbdata,'+' , data_two, size_two);if (textconv_one) -free ((char *)data_one); +free (data_one);if (textconv_two) -free ((char *)data_two); +free (data_two); }
These variables lost their const in 840383b2c2 (textconv:
refactor calls to run_textconv, 2010-04-01). OK, makes sense.
Quoted 5 lines
http.c +0
-0
http.c static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb, · line 2326
Quoted 10 lines
http.c +2
-2@@ -2319,8 +2319,8 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb, } do { - ssize_t retval = xwrite(freq->localfile, - (char *) ptr + posn, size - posn); + ssize_t retval = + xwrite(freq->localfile, ptr + posn, size - posn); if (retval < 0) return posn / eltsize; posn += retval;
This one went from a void pointer to a char pointer in a04ff3ec32 (http: make curl callbacks match contracts from curl header, 2011-05-03). Good.
imap-send.c static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd) · line 785
Quoted 13 lines
imap-send.c +1
-1@@ -779,7 +779,7 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd)if (cmdp->cb.data) { n =socket_write (&imap->buf.sock, cmdp->cb.data, cmdp->cb.dlen);FREE_AND_NULL (cmdp->cb.data); -if (n != (int )cmdp->cb.dlen) +if (n != cmdp->cb.dlen)return RESP_BAD ; }else if (cmdp->cb.cont) {if (cmdp->cb.cont (ctx, cmdp, cmd))
This one dates back to f2561fda36 (Add git-imap-send, derived from isync 1.0.1., 2006-03-10), so no clue how it came about. It's clearly pointless, though, as "dlen" is already an int.
imap-send.c int cmd_main(int argc, const char **argv) · line 1530
Quoted 7 lines
imap-send.c +2
-1@@ -1526,7 +1526,8 @@ int cmd_main(int argc, const char **argv)setup_git_directory_gently (&nongit_ok);git_config (git_imap_config,NULL ); - argc =parse_options (argc, (const char **)argv,"" , imap_send_options, imap_send_usage,0 ); + argc =parse_options (argc, argv,"" , imap_send_options, imap_send_usage, +0 );
This is another one that picked up a const when switching to cmd_main().
merge-ort.c static void apply_directory_rename_modifications(struct merge_options *opt, · line 2578
Quoted 11 lines
merge-ort.c +1
-1@@ -2574,7 +2574,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt,/* Find parent directories missing from opt->priv->paths */ cur_path =mem_pool_strdup (&opt->priv->pool, new_path); -free ((char *)new_path); +free (new_path); new_path = (char *)cur_path;
This one was always a "char *", going back to fa5e06d690 (merge-ort: modify collect_renames() for directory rename handling, 2021-01-19).
There's another variable in there, parent_name, which probably _should_ be non-const, since we allocate and free it. That's out of scope for your patch, though.
oidmap.h static inline void *oidmap_iter_first(struct oidmap *map, · line 91
Quoted 11 lines
oidmap.h +1
-1@@ -87,7 +87,7 @@ static inline void *oidmap_iter_first(struct oidmap *map, {oidmap_iter_init (map, iter);/* TODO: this API could be reworked to do compile-time type checks */ -return (void *)oidmap_iter_next (iter); +return oidmap_iter_next (iter); }
This comes from 87571c3f71 (hashmap: use *_entry APIs for iteration, 2019-10-06), whose only change to this line is adding the cast. Maybe the intent was to flag the area to be handled later according to the comment? I'd think the comment itself suffices, there. But if not, then we're undoing it.
There's an identical case (from the same commit) a few lines above. If we are changing this one, we should do both.
I'll stop here for now. It's a fair bit of leg-work digging these up (though again, I do think there's value in understanding why the cast was there, even if we know it isn't _currently_ doing anything).
-Peff
Quoted 4 lines · Jeff King
I'll stop here for now. It's a fair bit of leg-work digging these up (though again, I do think there's value in understanding why the cast was there, even if we know it isn't _currently_ doing anything).
I agree with the value of understanding why each of these casts has become unnecessary, and thanks for demonstrating how a rerolled version should justify its changes with its findings behind each of the unnecessary casts.
What do you recommend the next round should look like? Multi patch series, each of which removes one cast with its proposed log message explains how it has become unneeded? A single patch with a gigantic proposed log message that lists the findings for each and all casts that are removed? Somewhere in between, perhaps split along the file boundary, or grouped by the event that made them unneeded (e.g. "cmd_main() used to take non-const but when we made it to take const, all of these casts we remove in this patch became unneeded")?
Thanks.
Quoted 5 lines · Rose via GitGitGadget
From: Seija Kijin <[email protected]>
Many of these casts remain,
even though the target variable is the type it is being casted to. We can safely remove said casts.
Like Peff I curious how you found these. I also agree that it would be helpful to understand the history so we can be sure there the cast is not a sympton of a bug - Peff has given you a great start on that. A lot of the changes look like useful cleanups but there are a couple that caught my eye as being wrong or undesirable which I've commented on below.
strbuf.c void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags) · line 496
Quoted 10 lines
strbuf.c +1
-1@@ -493,7 +493,7 @@ void strbuf_add_percentencode(struct strbuf *dst, const char *src, int flags)if (ch <=0x1F || ch >=0x7F || (ch =='/' && (flags &STRBUF_ENCODE_SLASH )) ||strchr (URL_UNSAFE_CHARS , ch)) -strbuf_addf (dst,"%%%02X" , (unsigned char )ch); +strbuf_addf (dst,"%%%02X" , ch);
This one is wrong, the cast is required if char is signed as in that case it will be converted to a signed integer (because printf() takes a variable number of arguments) and then %X treats that integer as unsigned. That means that if the high bit is set we'll now print a bunch of leading F's. To see this compile and run
#include <stdio.h>
int main(void)
{
printf("%02X %02X\n", (signed char)0x80, (unsigned char)0x80);
return 0;
}which prints
FFFFFF80 80
xdiff/xprepare.c static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_ · line 191
Quoted 10 lines
xdiff/xprepare.c +1
-1@@ -188,7 +188,7 @@ static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_if (!(crec =xdl_cha_alloc (&xdf->rcha))) goto abort; crec->ptr = prev; - crec->size = (long ) (cur - prev); + crec->size = (cur - prev);
I'm not sure if we want to remove this cast. cur and prev are pointers so (cur - prev) has type ptrdiff_t which maybe wider than long. We can debate whether we should be using a cast to hide any compiler warning here but the cast is not redundant in the same way as others in this patch.
xdiff/xutils.c long xdl_guess_lines(mmfile_t *mf, long sample) { · line 133
Quoted 13 lines
xdiff/xprepare.c +0
-0crec->ha = hav; recs[nrec++] = crec;if (xdl_classify_record (pass, cf, rhash, hbits, crec) <0 )xdiff/xutils.c +1
-1@@ -130,7 +130,7 @@ long xdl_guess_lines(mmfile_t *mf, long sample) {else cur++; } - tsize += (long ) (cur - data); + tsize += (cur - data);
This is in the same class as the one above. (Also if we do end up removing the cast we should remove the parentheses as well)
Best Wishes
Phillip
Quoted 21 lines · Jeff King
On Wed, Dec 14, 2022 at 03:35:26PM +0000, Rose via GitGitGadget wrote:
[...]oidmap.h static inline void *oidmap_iter_first(struct oidmap *map, · line 91
Quoted 11 lines
oidmap.h +1
-1@@ -87,7 +87,7 @@ static inline void *oidmap_iter_first(struct oidmap *map, {oidmap_iter_init (map, iter);/* TODO: this API could be reworked to do compile-time type checks */ -return (void *)oidmap_iter_next (iter); +return oidmap_iter_next (iter); }This comes from
87571c3f71(hashmap: use*_entryAPIs for iteration, 2019-10-06), whose only change to this line is adding the cast. Maybe the intent was to flag the area to be handled later according to the comment? I'd think the comment itself suffices, there. But if not, then we're undoing it.
Quoted 7 lines
There's an identical case (from the same commit) a few lines above. If we are changing this one, we should do both.
I'll stop here for now. It's a fair bit of leg-work digging these up (though again, I do think there's value in understanding why the cast was there, even if we know it isn't _currently_ doing anything).
Thanks for doing all that leg-work. Whan I saw this patch earlier I was going to point out the same.
I.e. we really should know why these are there in the first place, or alterantively convince ourselves that e.g a wide-ranging coccinelle rule we're applying is safe.
I wondered if we could find potential candidates with a brute-force approach, this is very much a hack, but perhaps for something for Rose to chew on (either for this topic, or as a follow-up):
# Could adjust this regex to catch more types...
perl -pi -e 's/\((void|const char|char)\s*\*\)\s*//g' $(git ls-files '*.[ch]')
# Commit a hunk at a time...
while true; do echo y | git add -p && git commit -m"bump"; doneThis gives you around ~500 commits. It's also inaccurate, e.g. it ruined some function-prototypes, edited comments etc. But let's look past that for now.
Then, as even further brute-forcing, let's get rid of those commits generated above where the compiler doesn't like it:
git rebase -i -x 'make objects || git reset --hard HEAD^1 && make objects'
Now, I'd never suggest that we actually apply the result of this monkeypatching.
Even if the compiler could spot logic errors in this with 100% accuratecy the above would still be broken, as it won't compile all possible codepaths (e.g. in compat/*, which I should have left out to begin with).
But it *is* useful to see common patterns that might serve as potential candidates, and it even spots a case where e.g. the "mf.ptr" still has a cast in diff.c, so that seems to be missed from the upthread patch.
Rose: It's really much more useful to edit contrib/coccinelle/*.cocci to add a rule saying "whenever we have a type X being assigned to, cast to Y, that Y cast can be removed". The coccinelle engine is much better at spotting those than human eyes, or grep/search & replace.
Those disclaimers aside, the above yields ~170 commits on top of the above.
Excluding compat/ the diff is the below. Some of it's broken, but some of it suggests things that are worth picking up, e.g. the return value of xmalloc() being cast (a C++-ism), "(char *)NULL" (do we ever need to cast NULL?)
bisect.c | 2 +- block-sha1/sha1.c | 6 +++--- builtin/bisect.c | 14 +++++++------- builtin/help.c | 10 +++++----- builtin/receive-pack.c | 2 +- builtin/replace.c | 2 +- builtin/tag.c | 2 +- bundle-uri.c | 2 +- connect.c | 2 +- diff.c | 6 +++--- fsck.c | 2 +- git-compat-util.h | 16 ++++++++-------- grep.c | 2 +- hashmap.c | 2 +- http.c | 2 +- imap-send.c | 4 ++-- khash.h | 4 ++-- line-range.c | 4 ++-- list.h | 2 +- notes.c | 2 +- object-name.c | 2 +- oidmap.h | 2 +- pack-bitmap.c | 2 +- pack-revindex.c | 4 ++-- read-cache.c | 6 +++--- reftable/writer.c | 4 ++-- rerere.c | 6 +++--- resolve-undo.c | 2 +- revision.c | 2 +- sha1dc/sha1.c | 2 +- t/helper/test-rot13-filter.c | 4 ++-- t/helper/test-run-command.c | 4 ++-- t/helper/test-simple-ipc.c | 4 ++-- t/helper/test-string-list.c | 2 +- thread-utils.c | 2 +- thread-utils.h | 4 ++-- xdiff/xutils.c | 2 +- 37 files changed, 71 insertions(+), 71 deletions(-)
bisect.c +1 -1
@@ -1179 +1179 @@ int bisect_clean_state(void) -for_each_ref_in ("refs/bisect" , mark_for_removal, (void *) &refs_for_removal); +for_each_ref_in ("refs/bisect" , mark_for_removal, &refs_for_removal);
block-sha1/sha1.c +3 -3
@@ -196 +196 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, size_t len) -memcpy (lenW + (char *)ctx->W, data, left); +memcpy (lenW + ctx->W, data, left);
@@ -199 +199 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, size_t len) - data = ((const char *)data + left); + data = (data + left);
@@ -206 +206 @@ void blk_SHA1_Update(blk_SHA_CTX *ctx, const void *data, size_t len) - data = ((const char *)data +64 ); + data = (data +64 );
builtin/bisect.c +7 -7
@@ -79 +79 @@ static void set_terms(struct bisect_terms *terms, const char *bad, -free ((void *)terms->term_good); +free (terms->term_good);
@@ -81 +81 @@ static void set_terms(struct bisect_terms *terms, const char *bad, -free ((void *)terms->term_bad); +free (terms->term_bad);
@@ -419 +419 @@ static void bisect_status(struct bisect_state *state, - (void *) &state->nr_good); + &state->nr_good);
@@ -723 +723 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a -free ((void *) terms->term_good); +free (terms->term_good);
@@ -728 +728 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a -free ((void *) terms->term_good); +free (terms->term_good);
@@ -736 +736 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a -free ((void *) terms->term_bad); +free (terms->term_bad);
@@ -741 +741 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a -free ((void *) terms->term_bad); +free (terms->term_bad);
builtin/help.c +5 -5
@@ -275 +275 @@ static void exec_woman_emacs(const char *path, const char *page) -execlp (path,"emacsclient" ,"-e" , man_page.buf, (char *)NULL ); +execlp (path,"emacsclient" ,"-e" , man_page.buf,NULL );
@@ -307 +307 @@ static void exec_man_man(const char *path, const char *page) -execlp (path,"man" , page, (char *)NULL ); +execlp (path,"man" , page,NULL );
@@ -315 +315 @@ static void exec_man_cmd(const char *cmd, const char *page) -execl (SHELL_PATH ,SHELL_PATH ,"-c" , shell_cmd.buf, (char *)NULL ); +execl (SHELL_PATH ,SHELL_PATH ,"-c" , shell_cmd.buf,NULL );
@@ -505 +505 @@ static void show_info_page(const char *page) -execlp ("info" ,"info" ,"gitman" , page, (char *)NULL ); +execlp ("info" ,"info" ,"gitman" , page,NULL );
@@ -534 +534 @@ static void open_html(const char *path) -execl_git_cmd ("web--browse" ,"-c" ,"help.browser" , path, (char *)NULL ); +execl_git_cmd ("web--browse" ,"-c" ,"help.browser" , path,NULL );
builtin/receive-pack.c +1 -1
@@ -1714 +1714 @@ static void check_aliased_updates(struct command *commands) - item->util = (void *)cmd; + item->util = cmd;
builtin/replace.c +1 -1
@@ -94 +94 @@ static int list_replace_refs(const char *pattern, const char *format) -for_each_replace_ref (the_repository, show_reference, (void *)&data); +for_each_replace_ref (the_repository, show_reference, &data);
builtin/tag.c +1 -1
@@ -130 +130 @@ static int delete_tags(const char **argv) - result =for_each_tag_name (argv, collect_tags, (void *)&refs_to_delete); + result =for_each_tag_name (argv, collect_tags, &refs_to_delete);
bundle-uri.c +1 -1
@@ -21 +21 @@ static int compare_bundles(const void *hashmap_cmp_fn_data, -return strcmp (e1->id, id ? (const char *)id : e2->id); +return strcmp (e1->id, id ? id : e2->id);
connect.c +1 -1
@@ -219 +219 @@ static void annotate_refs_with_symref_info(struct ref *ref) - ref->symref =xstrdup ((char *)item->util); + ref->symref =xstrdup (item->util);
diff.c +3 -3
@@ -581 +581 @@ static int fill_mmfile(struct repository *r, mmfile_t *mf, - mf->ptr = (char *)"" ;/* does not matter */ + mf->ptr ="" ;/* does not matter */
@@ -4058 +4058 @@ int diff_populate_filespec(struct repository *r, - s->data = (char *)"" ; + s->data ="" ;
@@ -4201 +4201 @@ static void prep_temp_blob(struct index_state *istate, - (const char *)blob, (size_t)size, &buf, &meta)) { + blob, (size_t)size, &buf, &meta)) {
fsck.c +1 -1
@@ -749 +749 @@ static int verify_headers(const void *data, unsigned long size, -const char *buffer = (const char *)data; +const char *buffer = data;
git-compat-util.h +8 -8
@@ -89 +89 @@ struct strbuf; -* ((char *)(foo) \ +* ((foo) \
@@ -302 +302 @@ static inline int is_xplatform_dir_sep(int c) -* Size needs to match (or exceed) 'sizeof(void *)'. +* Size needs to match (or exceed) 'sizeof'.
@@ -355 +355 @@ static inline int setitimer(int which UNUSED, -char *gitbasename (char *); +char *gitbasename;
@@ -357 +357 @@ char *gitbasename(char *); -char *gitdirname (char *); +char *gitdirname;
@@ -868 +868 @@ int gitsetenv(const char *, const char *, int); -char *gitmkdtemp (char *); +char *gitmkdtemp;
@@ -873 +873 @@ char *gitmkdtemp(char *); -int gitunsetenv (const char *); +int gitunsetenv;
@@ -963 +963 @@ static inline char *gitstrchrnul(const char *s, int c) -return (char *)s; +return s;
@@ -1573 +1573 @@ int uncompress2(Bytef *dest, uLongf *destLen, const Bytef *source, - ((type *) ((char *)(ptr) - offsetof(type , member))) + ((type *) ((ptr) - offsetof(type , member)))
grep.c +1 -1
@@ -391 +391 @@ static void free_pcre2_pattern(struct grep_pat *p) -free ((void *)p->pcre2_tables); +free (p->pcre2_tables);
hashmap.c +1 -1
@@ -188 +188 @@ static void free_individual_entries(struct hashmap *map, ssize_t entry_offset) -free ((char *)e - entry_offset); +free (e - entry_offset);
http.c +1 -1
@@ -550 +550 @@ static int sockopt_callback(void *client, curl_socket_t fd, curlsocktype type) - rc =setsockopt (fd,SOL_SOCKET ,SO_KEEPALIVE , (void *)&ka, len); + rc =setsockopt (fd,SOL_SOCKET ,SO_KEEPALIVE , &ka, len);
imap-send.c +2 -2
@@ -777 +777 @@ static int get_cmd_result(struct imap_store *ctx, struct imap_cmd *tcmd) - cmdp = (struct imap_cmd *)((char *)imap->in_progress_append - + cmdp = (struct imap_cmd *)(imap->in_progress_append -
@@ -903 +903 @@ static char *cram(const char *challenge_64, const char *user, const char *pass) -return (char *)response_64; +return response_64;
khash.h +2 -2
@@ -88,2 +88,2 @@ static const double __ac_HASH_UPPER = 0.77; -free ((void *)h->keys); \ -free ((void *)h->vals); \ +free (h->keys); \ +free (h->vals); \
line-range.c +2 -2
@@ -160 +160 @@ static const char *find_funcname_matching_regexp(xdemitconf_t *xecfg, const char -if (match_funcname (xecfg, (char *) bol, (char *) eol)) +if (match_funcname (xecfg, bol, eol))
@@ -216 +216 @@ static const char *parse_range_funcname( - p =find_funcname_matching_regexp (xecfg, (char *) start, ®exp); + p =find_funcname_matching_regexp (xecfg, start, ®exp);
list.h +1 -1
@@ -122 +122 @@ static inline void list_splice(struct list_head *add, struct list_head *head) - ((type *) ((char *) (ptr) - offsetof(type , member))) + ((type *) ((ptr) - offsetof(type , member)))
notes.c +1 -1
@@ -830 +830 @@ int combine_notes_concatenate(struct object_id *cur_oid, - buf = (char *)xmalloc (buf_len); + buf =xmalloc (buf_len);
object-name.c +1 -1
@@ -808 +808 @@ int repo_find_unique_abbrev_r(struct repository *r, char *hex, - ds.cb_data = (void *)&mad; + ds.cb_data = &mad;
oidmap.h +1 -1
@@ -82 +82 @@ static inline void *oidmap_iter_next(struct oidmap_iter *iter) -return (void *)hashmap_iter_next (&iter->h_iter); +return hashmap_iter_next (&iter->h_iter);
pack-bitmap.c +1 -1
@@ -202 +202 @@ static int load_bitmap_header(struct bitmap_index *index) - index->table_lookup = (void *)(index_end - table_size); + index->table_lookup = (index_end - table_size);
pack-revindex.c +2 -2
@@ -278 +278 @@ static int load_pack_revindex_from_disk(struct packed_git *p) - p->revindex_data = (const uint32_t *)((const char *)p->revindex_map +RIDX_HEADER_SIZE ); + p->revindex_data = (const uint32_t *)(p->revindex_map +RIDX_HEADER_SIZE );
@@ -333 +333 @@ int load_midx_revindex(struct multi_pack_index *m) - m->revindex_data = (const uint32_t *)((const char *)m->revindex_map +RIDX_HEADER_SIZE ); + m->revindex_data = (const uint32_t *)(m->revindex_map +RIDX_HEADER_SIZE );
read-cache.c +3 -3
@@ -1920 +1920 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool, - name = (const char *)(flagsp +2 *sizeof (uint16_t )); + name = (flagsp +2 *sizeof (uint16_t ));
@@ -1923 +1923 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool, - name = (const char *)(flagsp +sizeof (uint16_t )); + name = (flagsp +sizeof (uint16_t ));
@@ -1980 +1980 @@ static struct cache_entry *create_from_disk(struct mem_pool *ce_mem_pool, - *ent_size = (name - ((char *)ondisk)) + len +1 - copy_len; + *ent_size = (name - (ondisk)) + len +1 - copy_len;
reftable/writer.c +2 -2
@@ -193 +193 @@ static void writer_index_hash(struct reftable_writer *w, struct strbuf *hash) - tree_search((void *)key, &w->obj_index_tree, + tree_search(key, &w->obj_index_tree,
@@ -280 +280 @@ int reftable_writer_add_ref(struct reftable_writer *w, - strbuf_add(&h, (char *)reftable_ref_record_val1(ref), + strbuf_add(&h, reftable_ref_record_val1(ref),
rerere.c +3 -3
@@ -556 +556 @@ static int find_conflict(struct repository *r, struct string_list *conflict) -string_list_insert (conflict, (const char *)e->name); +string_list_insert (conflict, e->name);
@@ -590 +590 @@ int rerere_remaining(struct repository *r, struct string_list *merge_rr) -string_list_insert (merge_rr, (const char *)e->name); +string_list_insert (merge_rr, e->name);
@@ -593 +593 @@ int rerere_remaining(struct repository *r, struct string_list *merge_rr) - it =string_list_lookup (merge_rr, (const char *)e->name); + it =string_list_lookup (merge_rr, e->name);
resolve-undo.c +1 -1
@@ -81 +81 @@ struct string_list *resolve_undo_read(const char *data, unsigned long size) - len = (endptr +1 ) - (char *)data; + len = (endptr +1 ) - data;
revision.c +1 -1
@@ -3909 +3909 @@ static int commit_match(struct commit *commit, struct rev_info *opt) - (char *)message, strlen(message)); + message, strlen(message));
sha1dc/sha1.c +1 -1
@@ -1873 +1873 @@ int SHA1DCFinal(unsigned char output[20], SHA1_CTX *ctx) -SHA1DCUpdate (ctx, (const char *)(sha1_padding), padn); +SHA1DCUpdate (ctx, (sha1_padding), padn);
t/helper/test-rot13-filter.c +2 -2
@@ -67,2 +67,2 @@ static char *get_value(char *buf, const char *key) - !skip_prefix ((const char *)buf, key, (const char **)&buf) || - !skip_prefix((const char *)buf, "=", (const char **)&buf) || + !skip_prefix (buf, key, (const char **)&buf) || + !skip_prefix(buf, "=", (const char **)&buf) ||
t/helper/test-run-command.c +2 -2
@@ -110 +110 @@ static int test_finished(int result, struct strbuf *err, void *cb, -const char *name = (const char *)task_cb; +const char *name = task_cb;
@@ -123 +123 @@ static int test_failed(struct strbuf *out, void *cb, void *task_cb) -const char *name = (const char *)task_cb; +const char *name = task_cb;
t/helper/test-simple-ipc.c +2 -2
@@ -181 +181 @@ static int test_app_cb(void *application_data, -if (application_data != (void *)&my_app_data) +if (application_data != &my_app_data)
@@ -269 +269 @@ static int daemon__run_server(void) - ret =ipc_server_run (cl_args.path, &opts, test_app_cb, (void *)&my_app_data); + ret =ipc_server_run (cl_args.path, &opts, test_app_cb, &my_app_data);
t/helper/test-string-list.c +1 -1
@@ -41 +41 @@ static int prefix_cb(struct string_list_item *item, void *cb_data) -const char *prefix = (const char *)cb_data; +const char *prefix = cb_data;
thread-utils.c +1 -1
@@ -89 +89 @@ int dummy_pthread_create(pthread_t *pthread, const void *attr, - void *(*fn)(void *), void *data) + void *(*fn), void *data)
thread-utils.h +2 -2
@@ -46 +46 @@ int dummy_pthread_create(pthread_t *pthread, const void *attr, - void *(*fn)(void *), void *data); + void *(*fn), void *data);
@@ -49 +49 @@ int dummy_pthread_join(pthread_t pthread, void **retval); -int dummy_pthread_init (void *); +int dummy_pthread_init;
xdiff/xutils.c +1 -1
@@ -115 +115 @@ void *xdl_cha_alloc(chastore_t *cha) { - data = (char *) ancur +sizeof (chanode_t) + ancur->icurr; + data = ancur +sizeof (chanode_t) + ancur->icurr;
Quoted 11 lines · Junio C Hamano
Quoted 4 lines · Jeff King
I'll stop here for now. It's a fair bit of leg-work digging these up (though again, I do think there's value in understanding why the cast was there, even if we know it isn't _currently_ doing anything).
I agree with the value of understanding why each of these casts has become unnecessary, and thanks for demonstrating how a rerolled version should justify its changes with its findings behind each of the unnecessary casts.
I almost didn't write my earlier email, because I was worried that I was creating work where it didn't need to exist (and all of the cases I looked at seemed like good changes). But the cases that Phillip found are a good reminder of the value of being careful here.
Quoted 8 lines
What do you recommend the next round should look like? Multi patch series, each of which removes one cast with its proposed log message explains how it has become unneeded? A single patch with a gigantic proposed log message that lists the findings for each and all casts that are removed? Somewhere in between, perhaps split along the file boundary, or grouped by the event that made them unneeded (e.g. "
cmd_main()used to take non-const but when we made it to take const, all of these casts we remove in this patch became unneeded")?
When I've done fixes across the code base like this in the past, I'd usually try to group them into patches by root cause. So perhaps:
- this used to be "char **argv" because of main(), but either because of becoming a builtin or cmd_main() it is now const
- this used to require casting from non-const to const to free()
- etc, with one-offs for cases that don't fit in any group
The keeps a single patch's explanation from being too overwhelming, and avoids repeating yourself when the same logic applies to multiple cases.
That said, I am happy with any solution that explains why we are sure each case is OK to do. :)
-Peff
Quoted 4 lines · Ævar Arnfjörð Bjarmason
Excluding compat/ the diff is the below. Some of it's broken, but some of it suggests things that are worth picking up, e.g. the return value of
xmalloc()being cast (a C++-ism), "(char *)NULL" (do we ever need to cast NULL?)
Yeah, getting rid of explicit casts to/from void (as in xmalloc()) is a good thing, IMHO.
Casting NULL is trickier. In normal assignment, no, it should never be needed. When passed to a variadic function (which covers all the cases in your patch below), the compiler needs to know that it's a pointer, and not the integer 0. So of the two allowed null pointer constants, only one is guaranteed to work:
execl(foo, 0); /* bad! there's nothing to say it's a pointer */ execl(foo, (char *)0); /* ok */
though whether it matters in practice depends on your ABI, I think.
But what about NULL? My copy of C99 (7.17.3) says it "expands to an implementation-defined null pointer constant". So it could be either of those.
And that is backed up by looking at "git log -S')NULL'", which yields 5d314759d7 (Cast execl*() NULL sentinels to (char *), 2010-07-24).
That said, I think we have lots of bare NULLs passed to variadic functions. Any function with LAST_ARG_MUST_BE_NULL will have a NULL in each of its callers, and we do not bother casting most of them. So I think this is one of those "technically could violate the standard, but OK in practice" things.
-Peff
Quoted 11 lines · Jeff King
... So perhaps:
- this used to be "char **argv" because of
main(), but either because of becoming a builtin orcmd_main()it is now const- this used to require casting from non-const to const to
free()- etc, with one-offs for cases that don't fit in any group
The keeps a single patch's explanation from being too overwhelming, and avoids repeating yourself when the same logic applies to multiple cases.
Thanks, that would be the organization I would use, too, if I were doing this change myself.
Quoted 2 lines
That said, I am happy with any solution that explains why we are sure each case is OK to do. :)
Yup.