Re: [PATCH] Fixes: Fallthrough warning on line 270 of qemu/qapi/opts-visitor.c

[Rohit Shinde]

Hey Philippe,

Thanks for the detailed comments! I have a couple of questions.

1. I'll modify the patch to just include a fallthrough comment instead of an attribute. How do I include the v4 version number in the patch? Do I erase the last commit on my branch or fork from the master and start the work again and label it as v4?
2. I am trying to find some issues of interest, starting through which I can go to bigger contributions. Do you have any suggestions on how I might do that? For now, I am trying to tackle the bite sized issues to find my way around the code base. I would like to move to substantial contributions.
3. I have a background in CS theory, but its been 2 years since I graduated from my Master's so I am a bit rusty on some stuff. How much CS theory (like compilers and OS) do I need to know if I want to contribute?

Thanks,

Rohit.

On Sat, Aug 15, 2020 at 11:24 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

Hi Rohit,

Congratulation for your first patch! It is in very
good shape already :)

It is easier for the reviewers if you start the patch subject with
the name of the subsystem concerned, or the file modified:

"qapi/opts-visitor: Add missing fallthrough annotations"

On 8/15/20 3:00 PM, Rohit Shinde wrote:
> Added the fallthrough comment so that the compiler doesn't emit an error on compiling with the -Wimplicit-fallthrough flag.

If possible align the description to 72 chars.

>
> Signed off by: Rohit Shinde

The tag is written "Signed-off-by" with '-', then your "name <email>":

Signed-off-by: Rohit Shinde <rohit.shinde12194@gmail.com>

If you configure your git client, using 'git-commit -s' will
automatically add the S-o-b tag:

$ git config user.name "Rohit Shinde"
$ git config user.email "rohit.shinde12194@gmail.com"
$ git commit -s

> ---
>  qapi/opts-visitor.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
> index 7781c23a42..43cf60d3a0 100644
> --- a/qapi/opts-visitor.c
> +++ b/qapi/opts-visitor.c
> @@ -266,6 +266,7 @@ opts_next_list(Visitor *v, GenericList *tail, size_t size)
>          }
>          ov->list_mode = LM_IN_PROGRESS;
>          /* range has been completed, fall through in order to pop option */
> +        __attribute__((fallthrough));

C uses attributes when declaring a type/variable/function.
Here this is inside a function body, not a declaration.
A simple "/* fallthrough */" comment will make the compiler happy.
You can see a similar patch for example:
https://git.qemu.org/?p=qemu.git;a=blobdiff;f=disas/sh4.c;h=dcdbdf26d8;hp=55ef865a3;hb=ccb237090f;hpb=7aa12aa215

When you find an issue that might have already been fixed elsewhere
in the repository, 'git-log -p' is your friend. Since the commits are
patches already accepted/merged, they might be a good source to learn
(how the issue was fixed, how the bug was described, ...). 

Regards,

Phil.

> 
>      case LM_IN_PROGRESS: {
>          const QemuOpt *opt;
>