Re: [PATCH v2 2/3] docs: clarify cmd_psuh signature and explain UNUSED macro
K Jayatheerth <jayatheerthkulkarni2005@gmail.com> writes:
> The documentation previously omitted the UNUSED macro,
> which often led to confusion for new contributors
> when they encountered compiler warnings related to unused parameters.
The above is not quite easy to reason about. It is more like we wrote this document, and then later tightened the default compiler warnings for developer builds. So "omitted" may technically be correct, but it was more like "did not use it, because there was no need".
The sample program, as written, would not build for at least two
reasons: - Since this document was first written, the calling convention
to subcommand implementation has changed, and now cmd_psuh()
needs to accept the third parameter, repository. - These days, compiler warning options for developers include
one that detects and complains about unused parameters, so
ones that are deliberately unused have to be marked as such.After such observation on the status quo and description of the problem you are going to solve, you give an order to the code base to fix it, perhaps like:
Update the old-style examples to adjust to the current
practices, with explanations as needed.To recap, the usual way to compose a log message of this project is to
- Give an observation on how the current system works in the
present tense (so no need to say "Currently X is Y", or
"Previously X was Y" to describe the state before your change;
just "X is Y" is enough), and discuss what you perceive as a
problem in it.
- Propose a solution (optional---often, problem description
trivially leads to an obvious solution in reader's minds).
- Give commands to the codebase to "become like so".
in this order.
Show 24 quoted lines
>
> Signed-off-by: K Jayatheerth <jayatheerthkulkarni2005@gmail.com>
> ---
> Documentation/MyFirstContribution.adoc | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/MyFirstContribution.adoc b/Documentation/MyFirstContribution.adoc
> index ef190d8748..f4320d8869 100644
> --- a/Documentation/MyFirstContribution.adoc
> +++ b/Documentation/MyFirstContribution.adoc
> @@ -142,7 +142,15 @@ command in `builtin/psuh.c`. Create that file, and within it, write the entry
> point for your command in a function matching the style and signature:
>
> ----
> -int cmd_psuh(int argc, const char **argv, const char *prefix)
> +int cmd_psuh(int argc, const char **argv, const char *prefix, struct repository *repo)
> +----
> +
> +We will use the UNUSED macro to make sure we don't recieve compiler warnings
> +for unused arguments from the function cmd_psuh.
> +----
> +int cmd_psuh(int argc UNUSED, const char **argv UNUSED,
> + const char *prefix UNUSED, struct repository *repo UNUSED)
> ----
I do not quite understand. Why do we need a new one here? Wouldn't it be easier to read for a newcomer if you just give the last one and explain what UNUSED are for? Perhaps like
... matching the style and signature:
----
int cmd_psuh(int argc UNUSED, const char **argv UNUSED,
const char *prefix UNUSED, struct repository *repo UNUSED)
---- A few things to note:
* A subcommand implementation takes its command line arguments
in `int argc` + `const char **argv`, like `main()` would * It also takes two extra parameters, `prefix` and `repo`. What
they mean will not be discussed until much later. * Because this first example will not use any of the parameters,
your compiler will give warnings on unused parameters. As the
list of these four parameters is mandated by the API to add
new built-in commands, you cannot omit them. Instead, you add
`UNUSED` to each of them to tell the compiler that you _know_
you are not (yet) using it.Take a special note on the last one. There may be multiple ways to squelch warnings, but it is worth telling your readers that use of UNUSED is the right way.
I'll stop here for this patch.
Thanks.
K Jayatheerth
