headers: name the arguments the way the definitions name them - #22550
karpovantonme wants to merge 3 commits into
Conversation
Eleven arguments carry one name in the declaration and another in the definition. The compiler does not mind, only the types have to match, but the header is what gets read first and the source is where the answer is. curl_dbg_malloc size -> wantedsize curl_dbg_calloc n, size -> wanted_elements, wanted_size curl_dbg_realloc size -> wantedsize curl_dbg_strdup src -> source r_freeaddrinfo res -> cahead my_get_line fp, db -> input, buf curl_slist_append_ccsid l -> list curl_version_info_ccsid cid -> ccsid curl_easy_setopt_ccsid curl -> easy Only the declarations move, the code is untouched.
|
Analysis of PR #22550 at f53aca09: Test 3017 failed, but it has been 53.9% flaky lately, so it's probably NOT a fault of the PR. Note that this test has failed in 2 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (2, to be specific) so it may be that this failure is rather a systemic issue with this job and not with this specific PR. Generated by Testclutch |
| CURL_EXTERN curl_version_info_data * | ||
| curl_version_info_ccsid(CURLversion stamp, unsigned int ccsid); |
There was a problem hiding this comment.
| CURL_EXTERN curl_version_info_data * | |
| curl_version_info_ccsid(CURLversion stamp, unsigned int ccsid); | |
| CURL_EXTERN curl_version_info_data *curl_version_info_ccsid( | |
| CURLversion stamp, | |
| unsigned int ccsid); |
Please use this format to stick with clang-format and rest of code.
| curl_formget_callback append, | ||
| unsigned int ccsid); | ||
| CURL_EXTERN CURLcode curl_easy_setopt_ccsid(CURL *curl, CURLoption tag, ...); | ||
| CURL_EXTERN CURLcode curl_easy_setopt_ccsid(CURL *easy, CURLoption tag, ...); |
There was a problem hiding this comment.
I suggest renaming in the definition instead to curl, because we prefer this
name for the CURL * type, if possible.
Review feedback: curl_easy_setopt_ccsid keeps `curl` in the header since that is the preferred name for the CURL * type, and the definition follows it instead. curl_version_info_ccsid rewrapped the way clang-format wants.
| unsigned int ccsid; | ||
| curl_off_t pfsize; | ||
| struct Curl_easy *data = easy; | ||
| struct Curl_easy *data = curl; |
There was a problem hiding this comment.
Needs renaming the remainingeasy references in the function, too. (this is not CI-tested)
There was a problem hiding this comment.
Maybe the fix was not pushed? easy references remain in line 1207, 1244 and more.
|
Thanks @vszakats both applied. Kept curl in the header for curl_easy_setopt_ccsid and renamed in the definition instead, and rewrapped curl_version_info_ccsid the way you suggested |
| unsigned int ccsid; | ||
| curl_off_t pfsize; | ||
| struct Curl_easy *data = easy; | ||
| struct Curl_easy *data = curl; |
There was a problem hiding this comment.
Maybe the fix was not pushed? easy references remain in line 1207, 1244 and more.
The rename reached the signature and the Curl_easy assignment, but five call sites in the body still passed `easy`, which no longer exists there. OS400 is not built in CI, so nothing caught it. Reported-by: Viktor Szakats
There was a problem hiding this comment.
Pull request overview
Aligns function parameter names between declarations and definitions without changing behavior.
Changes:
- Synchronizes parameter names across internal and OS/400 headers.
- Renames the OS/400
curl_easy_setopt_ccsidimplementation parameter to match its declaration. - Rewraps a declaration to preserve line-length limits.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/tool_parsecfg.h |
Aligns my_get_line parameter names. |
projects/OS400/ccsidcurl.h |
Aligns CCSID API parameter names. |
projects/OS400/ccsidcurl.c |
Aligns curl_easy_setopt_ccsid with its prototype. |
lib/fake_addrinfo.h |
Aligns r_freeaddrinfo parameter naming. |
lib/curl_setup.h |
Aligns memory-debug function parameter names. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
May be of interest, that clang-tidy's Merged now, thanks @karpovantonme. |
so, eleven arguments are named one way in the header and another in the definition. the compiler only checks types so nothing is broken, but the header is what people read first, and then the name changes under them in the source
only the declarations move, the code is untouched. happy to flip any of them the other way if you prefer the header name
oh and one line in ccsidcurl.h had to be rewrapped to stay under 79 columns after the rename. checksrc is clean
btw found with a checker I wrote with the help of Claude Opus 5, which pairs every prototype with its own definition. I read each of these in the source before touching it