Re: [Outreachy kernel] [PATCH] Edit CamelCase function names to make code more readable

[Julia Lawall <julia.lawall@lip6.fr>]

[-- Attachment #1: Type: text/plain, Size: 3686 bytes --]



On Sun, 17 Mar 2019, Vatsala Narang wrote:

>
> [PATCH] EDIT CAMELCASE FUNCTION NAMES TO MAKE CODE MORE READABLE

Please don't use all capital letters for the log message.  Also, you don't
need to put [PATCH] in the log message.

On the other hand, the subject line can be more concise.  Just "eliminate
camel case" could be fine.

The subject line also needs to give some information about the affected
files.  You can use

git log --oneline drivers/staging/sm750fb/ddk750_display.c

to find something suitable.  Don't try to make it up.  Do what others have
done.

> Issue reported by checkpatch.pl tool
>
> Signed-off-by:vatsalanarang<vatsalanarang@gmail.com>

This should be your full name, with normal capitalization and spacing.
You can upadte your git configuration file with this information, and then
you don't have to think about it any more.

> ---
>  drivers/staging/sm750fb/ddk750_display.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/sm750fb/ddk750_display.c
> b/drivers/staging/sm750fb/ddk750_display.c
> index f38051eedb6c..7f28e4cc4c79 100644
> --- a/drivers/staging/sm750fb/ddk750_display.c
> +++ b/drivers/staging/sm750fb/ddk750_display.c
> @@ -85,7 +85,7 @@ static void primary_wait_vertical_sync(int delay)
>         }
>  }
>
> -static void swPanelPowerSequence(int disp, int delay)
> +static void sw_panel_power_sequence(int disp, int delay)
>  {
>         unsigned int reg;
>
> @@ -111,7 +111,7 @@ static void swPanelPowerSequence(int disp, int delay)
>         primary_wait_vertical_sync(delay);
>  }
>
> -void ddk750_setLogicalDispOut(enum disp_output output)
> +void ddk750_set_logical_disp_out(enum disp_output output)

The above changes look fine.

>  {
>         unsigned int reg;
>
> @@ -147,12 +147,12 @@ void ddk750_setLogicalDispOut(enum disp_output output)
>
>         if (output & PNL_SEQ_USAGE) {
>                 /* set  panel sequence */
> -               swPanelPowerSequence((output & PNL_SEQ_MASK) >>
> PNL_SEQ_OFFSET,
> -                                    4);
> +               sw_panel_power_sequence
> +               ((output & PNL_SEQ_MASK) >> PNL_SEQ_OFFSET, 4);

Please don't do this.  Function calls should have the ( on the same line
as the function name, to be quickly understandable.  It may be that going
a few characters over 80 is the best solution in this case.

>         }
>
>         if (output & DAC_USAGE)
> -               setDAC((output & DAC_MASK) >> DAC_OFFSET);
> +               set_DAC((output & DAC_MASK) >> DAC_OFFSET);

If you change a call without changing the definition, the code is not
going to compile.

Furthermore, above you changed a definition but made no changes on the
uses of the function.  Is the function never used?

If you change the name of a defined function, then you have to update the
name at all of the call sites, in the same patch.  No patch should break
the build.

julia

>         if (output & DPMS_USAGE)
>                 ddk750_set_dpms((output & DPMS_MASK) >> DPMS_OFFSET);
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/434bf516-9e31-4dcf-ab81-
> 32842a588aee%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
>