| Message ID | pull.1396.git.git.1671032126602.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git: remove unneeded casts | expand |
On Wed, Dec 14, 2022 at 03:35:26PM +0000, Rose via GitGitGadget wrote: > From: Seija Kijin <doremylover123@gmail.com> > > 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). > diff --git a/builtin/credential-store.c b/builtin/credential-store.c > index 62a4f3c2653..93e521af395 100644 > --- a/builtin/credential-store.c > +++ b/builtin/credential-store.c > @@ -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. > diff --git a/diff.c b/diff.c > index 1054a4b7329..2e58c9372b4 100644 > --- a/diff.c > +++ b/diff.c > @@ -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); > } These variables lost their const in 840383b2c2 (textconv: refactor calls to run_textconv, 2010-04-01). OK, makes sense. > diff --git a/http.c b/http.c > index 8a5ba3f4776..32db5d76a7c 100644 > --- a/http.c > +++ b/http.c > @@ -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. > diff --git a/imap-send.c b/imap-send.c > index a50af56b827..e67dbfc5567 100644 > --- a/imap-send.c > +++ b/imap-send.c > @@ -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. > @@ -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(). > diff --git a/merge-ort.c b/merge-ort.c > index d1611ca400a..a2aefd609ad 100644 > --- a/merge-ort.c > +++ b/merge-ort.c > @@ -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. > diff --git a/oidmap.h b/oidmap.h > index c66a83ab1d6..9cefbba550d 100644 > --- a/oidmap.h > +++ b/oidmap.h > @@ -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
Jeff King <peff@peff.net> writes: > 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.
On Wed, Dec 14 2022, Jeff King wrote: > On Wed, Dec 14, 2022 at 03:35:26PM +0000, Rose via GitGitGadget wrote: > [...] >> diff --git a/oidmap.h b/oidmap.h >> index c66a83ab1d6..9cefbba550d 100644 >> --- a/oidmap.h >> +++ b/oidmap.h >> @@ -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). 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"; done This 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(-) diff --git a/bisect.c b/bisect.c index ec7487e6836..440098991c3 100644 --- a/bisect.c +++ b/bisect.c @@ -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); diff --git a/block-sha1/sha1.c b/block-sha1/sha1.c index 80cebd27564..9759febb120 100644 --- a/block-sha1/sha1.c +++ b/block-sha1/sha1.c @@ -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); diff --git a/builtin/bisect.c b/builtin/bisect.c index cc9483e8515..50e5275efaa 100644 --- a/builtin/bisect.c +++ b/builtin/bisect.c @@ -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); diff --git a/builtin/help.c b/builtin/help.c index 53f2812dfb1..c42d4763994 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -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); diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index a90af303630..bc229f112e0 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1714 +1714 @@ static void check_aliased_updates(struct command *commands) - item->util = (void *)cmd; + item->util = cmd; diff --git a/builtin/replace.c b/builtin/replace.c index a29e911d309..3a3bbb49d6d 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -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); diff --git a/builtin/tag.c b/builtin/tag.c index d428c45dc8d..8c0be18cb11 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -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); diff --git a/bundle-uri.c b/bundle-uri.c index 79a914f961b..3813935ec69 100644 --- a/bundle-uri.c +++ b/bundle-uri.c @@ -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); diff --git a/connect.c b/connect.c index 5ea53deda23..52bf52f2f91 100644 --- a/connect.c +++ b/connect.c @@ -219 +219 @@ static void annotate_refs_with_symref_info(struct ref *ref) - ref->symref = xstrdup((char *)item->util); + ref->symref = xstrdup(item->util); diff --git a/diff.c b/diff.c index 2e58c9372b4..6f0951350bf 100644 --- a/diff.c +++ b/diff.c @@ -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)) { diff --git a/fsck.c b/fsck.c index b3da1d68c0b..4183ca065c1 100644 --- a/fsck.c +++ b/fsck.c @@ -749 +749 @@ static int verify_headers(const void *data, unsigned long size, - const char *buffer = (const char *)data; + const char *buffer = data; diff --git a/git-compat-util.h b/git-compat-util.h index a76d0526f79..cf0e275621e 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -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))) diff --git a/grep.c b/grep.c index 06eed694936..de7c28776fb 100644 --- a/grep.c +++ b/grep.c @@ -391 +391 @@ static void free_pcre2_pattern(struct grep_pat *p) - free((void *)p->pcre2_tables); + free(p->pcre2_tables); diff --git a/hashmap.c b/hashmap.c index cf5fea87eb0..4a6a0d6b14a 100644 --- a/hashmap.c +++ b/hashmap.c @@ -188 +188 @@ static void free_individual_entries(struct hashmap *map, ssize_t entry_offset) - free((char *)e - entry_offset); + free(e - entry_offset); diff --git a/http.c b/http.c index 32db5d76a7c..3a8ece892ed 100644 --- a/http.c +++ b/http.c @@ -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); diff --git a/imap-send.c b/imap-send.c index e67dbfc5567..a73471300d2 100644 --- a/imap-send.c +++ b/imap-send.c @@ -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; diff --git a/khash.h b/khash.h index cb79bf88567..95ac4f4d9ab 100644 --- a/khash.h +++ b/khash.h @@ -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); \ diff --git a/line-range.c b/line-range.c index 955a8a95355..8ab64d95e7b 100644 --- a/line-range.c +++ b/line-range.c @@ -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); diff --git a/list.h b/list.h index 362a4cd7f5f..bab41d6aa64 100644 --- a/list.h +++ b/list.h @@ -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))) diff --git a/notes.c b/notes.c index f2805d51bb1..ffe2f030a56 100644 --- a/notes.c +++ b/notes.c @@ -830 +830 @@ int combine_notes_concatenate(struct object_id *cur_oid, - buf = (char *) xmalloc(buf_len); + buf = xmalloc(buf_len); diff --git a/object-name.c b/object-name.c index 2dd1a0f56e1..9908d65af17 100644 --- a/object-name.c +++ b/object-name.c @@ -808 +808 @@ int repo_find_unique_abbrev_r(struct repository *r, char *hex, - ds.cb_data = (void *)&mad; + ds.cb_data = &mad; diff --git a/oidmap.h b/oidmap.h index 9cefbba550d..c72e9c9c40e 100644 --- a/oidmap.h +++ b/oidmap.h @@ -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); diff --git a/pack-bitmap.c b/pack-bitmap.c index d2a42abf28c..3a3115ad105 100644 --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -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); diff --git a/pack-revindex.c b/pack-revindex.c index fa897b54584..ffe034957fc 100644 --- a/pack-revindex.c +++ b/pack-revindex.c @@ -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); diff --git a/read-cache.c b/read-cache.c index f4c4cc63dc4..0f1a18ecf02 100644 --- a/read-cache.c +++ b/read-cache.c @@ -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; diff --git a/reftable/writer.c b/reftable/writer.c index 2e322a5683d..ee015525536 100644 --- a/reftable/writer.c +++ b/reftable/writer.c @@ -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), diff --git a/rerere.c b/rerere.c index 876ab435da9..9b59fa7940b 100644 --- a/rerere.c +++ b/rerere.c @@ -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); diff --git a/resolve-undo.c b/resolve-undo.c index e81096e2d45..176186d7e9d 100644 --- a/resolve-undo.c +++ b/resolve-undo.c @@ -81 +81 @@ struct string_list *resolve_undo_read(const char *data, unsigned long size) - len = (endptr + 1) - (char*)data; + len = (endptr + 1) - data; diff --git a/revision.c b/revision.c index c86e76e4716..93ef97e7e76 100644 --- a/revision.c +++ b/revision.c @@ -3909 +3909 @@ static int commit_match(struct commit *commit, struct rev_info *opt) - (char *)message, strlen(message)); + message, strlen(message)); diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c index dede2cbddf9..901aff46cfb 100644 --- a/sha1dc/sha1.c +++ b/sha1dc/sha1.c @@ -1873 +1873 @@ int SHA1DCFinal(unsigned char output[20], SHA1_CTX *ctx) - SHA1DCUpdate(ctx, (const char*)(sha1_padding), padn); + SHA1DCUpdate(ctx, (sha1_padding), padn); diff --git a/t/helper/test-rot13-filter.c b/t/helper/test-rot13-filter.c index f8d564c622a..43ae901fd2c 100644 --- a/t/helper/test-rot13-filter.c +++ b/t/helper/test-rot13-filter.c @@ -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) || diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index eadde7c47b1..0d93ee1bcd7 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -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; diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c index 28365ff85b6..7830353e08d 100644 --- a/t/helper/test-simple-ipc.c +++ b/t/helper/test-simple-ipc.c @@ -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); diff --git a/t/helper/test-string-list.c b/t/helper/test-string-list.c index 2123dda85bf..43bea3ba6b3 100644 --- a/t/helper/test-string-list.c +++ b/t/helper/test-string-list.c @@ -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; diff --git a/thread-utils.c b/thread-utils.c index 6d3de821473..d00d018759b 100644 --- a/thread-utils.c +++ b/thread-utils.c @@ -89 +89 @@ int dummy_pthread_create(pthread_t *pthread, const void *attr, - void *(*fn)(void *), void *data) + void *(*fn), void *data) diff --git a/thread-utils.h b/thread-utils.h index 4961487ed91..e103cf82b0c 100644 --- a/thread-utils.h +++ b/thread-utils.h @@ -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; diff --git a/xdiff/xutils.c b/xdiff/xutils.c index 853f2260a1d..a6a6277fb52 100644 --- a/xdiff/xutils.c +++ b/xdiff/xutils.c @@ -115 +115 @@ void *xdl_cha_alloc(chastore_t *cha) { - data = (char *) ancur + sizeof(chanode_t) + ancur->icurr; + data = ancur + sizeof(chanode_t) + ancur->icurr;
On 14/12/2022 15:35, Rose via GitGitGadget wrote: > From: Seija Kijin <doremylover123@gmail.com> > > 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. > diff --git a/strbuf.c b/strbuf.c > index 0890b1405c5..940f59473eb 100644 > --- a/strbuf.c > +++ b/strbuf.c > @@ -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 > diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c > index c84549f6c50..04fa4e5a01d 100644 > --- a/xdiff/xprepare.c > +++ b/xdiff/xprepare.c > @@ -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. > crec->ha = hav; > recs[nrec++] = crec; > if (xdl_classify_record(pass, cf, rhash, hbits, crec) < 0) > diff --git a/xdiff/xutils.c b/xdiff/xutils.c > index 9e36f24875d..853f2260a1d 100644 > --- a/xdiff/xutils.c > +++ b/xdiff/xutils.c > @@ -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
On Thu, Dec 15, 2022 at 08:49:45AM +0900, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > 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. > 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
On Thu, Dec 15, 2022 at 11:07:24AM +0100, Ævar Arnfjörð Bjarmason wrote: > 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
Jeff King <peff@peff.net> writes: > ... 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. Thanks, that would be the organization I would use, too, if I were doing this change myself. > That said, I am happy with any solution that explains why we are sure > each case is OK to do. :) Yup.
diff --git a/builtin/credential-store.c b/builtin/credential-store.c index 62a4f3c2653..93e521af395 100644 --- a/builtin/credential-store.c +++ b/builtin/credential-store.c @@ -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 --git a/diff.c b/diff.c index 1054a4b7329..2e58c9372b4 100644 --- a/diff.c +++ b/diff.c @@ -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 { diff --git a/http.c b/http.c index 8a5ba3f4776..32db5d76a7c 100644 --- a/http.c +++ b/http.c @@ -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; diff --git a/imap-send.c b/imap-send.c index a50af56b827..e67dbfc5567 100644 --- a/imap-send.c +++ b/imap-send.c @@ -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); diff --git a/merge-ort.c b/merge-ort.c index d1611ca400a..a2aefd609ad 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -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) { diff --git a/oidmap.h b/oidmap.h index c66a83ab1d6..9cefbba550d 100644 --- a/oidmap.h +++ b/oidmap.h @@ -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 diff --git a/pack-revindex.c b/pack-revindex.c index 08dc1601679..fa897b54584 100644 --- a/pack-revindex.c +++ b/pack-revindex.c @@ -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; diff --git a/read-cache.c b/read-cache.c index 1ff518b2a7f..f4c4cc63dc4 100644 --- a/read-cache.c +++ b/read-cache.c @@ -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) diff --git a/ref-filter.c b/ref-filter.c index caf10ab23eb..e5993f7cc43 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -396,7 +396,7 @@ static int trailers_atom_parser(struct ref_format *format, struct used_atom *ato strbuf_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; } } diff --git a/strbuf.c b/strbuf.c index 0890b1405c5..940f59473eb 100644 --- a/strbuf.c +++ b/strbuf.c @@ -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); } diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c index 506835521a4..a788196adf5 100644 --- a/t/helper/test-parse-options.c +++ b/t/helper/test-parse-options.c @@ -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; diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c index f69709d674f..eb7a204f3b7 100644 --- a/t/helper/test-path-utils.c +++ b/t/helper/test-path-utils.c @@ -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++) diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index 3ecb830f4a8..eadde7c47b1 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -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; diff --git a/t/helper/test-subprocess.c b/t/helper/test-subprocess.c index ff22f2fa2c5..32a5b339eae 100644 --- a/t/helper/test-subprocess.c +++ b/t/helper/test-subprocess.c @@ -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); } diff --git a/thread-utils.c b/thread-utils.c index 53298456913..6d3de821473 100644 --- a/thread-utils.c +++ b/thread-utils.c @@ -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 diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c index c84549f6c50..04fa4e5a01d 100644 --- a/xdiff/xprepare.c +++ b/xdiff/xprepare.c @@ -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) diff --git a/xdiff/xutils.c b/xdiff/xutils.c index 9e36f24875d..853f2260a1d 100644 --- a/xdiff/xutils.c +++ b/xdiff/xutils.c @@ -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)