[U-Boot] [PATCH v3] moveconfig: add a second pass for empty #if/#endif blocks

[Chris Packham]

On Thu, 31 Jan 2019 19:26 Masahiro Yamada <yamada.masahiro at socionext.com
wrote:

> On Wed, Jan 30, 2019 at 5:55 PM Masahiro Yamada
> <yamada.masahiro at socionext.com> wrote:
> >
> > On Wed, Jan 30, 2019 at 4:23 PM Chris Packham <judge.packham at gmail.com>
> wrote:
> > >
> > > Moveconfig already attempts to remove empty #if/#endif blocks when
> there
> > > is a matching CONFIG_ being moved. Add a second pass which covers files
> > > without a match.
> > >
> > > Signed-off-by: Chris Packham <judge.packham at gmail.com>
> > > ---
> > > This was previously submitted as
> > > http://patchwork.ozlabs.org/patch/924901/ there still seems to be
> cases
> > > of #if/#endif left over from Kconfig migrations so perhaps this is
> still
> > > needed/wanted.
> > >
> > > I've plumbed this in as a second pass because ultimately we may want to
> > > make this a separate option. Also I couldn't figure out how to
> implement
> > > this without using re.M so I couldn't make it work in with the line by
> > > line parsing of cleanup_one_header().
> >
> >
> > This seems useful to find leftover code
> > regardless of the CONFIG options you are moving.
> >
> > One drawback is, it may fold unrelated cleanups.
> >
> > Maybe, it would be useful to add a separate option to turn on this
> feature,
> > or make it into a separate tool.
> >
> >
> >
>
>
> I tested this patch.
> It detected one empty block.
>
>
> diff --git a/include/configs/omap3_cairo.h b/include/configs/omap3_cairo.h
> index 04bce2f..ac1a6cb 100644
> --- a/include/configs/omap3_cairo.h
> +++ b/include/configs/omap3_cairo.h
> @@ -197,8 +197,6 @@
>   * are needed and peripheral clocks for UART2 must be enabled in
>   * function per_clocks_enable().
>   */
> -#ifdef CONFIG_SPL_BUILD
> -#endif
>
>  /* Provide the MACH_TYPE value the vendor kernel requires */
>  #define CONFIG_MACH_TYPE       3063
>
>
>
>
>
>
>
> This seems a leftover of 9baa2bce.
>

Yes. I sent a patch for this as I was testing.


>
> $ git show 9baa2bce -- include/configs/omap3_cairo.h
> commit 9baa2bce28901321d6f62399b5ebeb3fcb8e8a57
> Author: Adam Ford <aford173 at gmail.com>
> Date:   Tue Aug 7 07:08:32 2018 -0500
>
>     Removed unused references to CONFIG_SERIALx
>
>     After creating CONS_INDEX and migrating a bunch of boards to it,
>     there are a bunch of defined references to CONFIG_SERIALx which
>     are not referenced in any C code or #ifdef, so they can now be
>     removed
>
>     Signed-off-by: Adam Ford <aford173 at gmail.com>
>
> diff --git a/include/configs/omap3_cairo.h b/include/configs/omap3_cairo.h
> index 0133416..04bce2f 100644
> --- a/include/configs/omap3_cairo.h
> +++ b/include/configs/omap3_cairo.h
> @@ -198,8 +198,6 @@
>   * function per_clocks_enable().
>   */
>  #ifdef CONFIG_SPL_BUILD
> -#undef CONFIG_SERIAL3
> -#define CONFIG_SERIAL2
>  #endif
>
>  /* Provide the MACH_TYPE value the vendor kernel requires */
>
>
>
>
>
> However, I doubt it used the moveconfig tool.
>
>
> I re-ran the tool against the previous commit.
>
>
> $ git checkout 9baa2bce2890^
> $ tools/moveconfig.py -H SERIAL0 SERIAL1 SERIAL2 SERIAL3
> Clean up headers? [y/n]: y
> y
> ...
> $ git diff -- include/configs/omap3_cairo.h
> diff --git a/include/configs/omap3_cairo.h b/include/configs/omap3_cairo.h
> index 0133416..ac1a6cb 100644
> --- a/include/configs/omap3_cairo.h
> +++ b/include/configs/omap3_cairo.h
> @@ -197,10 +197,6 @@
>   * are needed and peripheral clocks for UART2 must be enabled in
>   * function per_clocks_enable().
>   */
> -#ifdef CONFIG_SPL_BUILD
> -#undef CONFIG_SERIAL3
> -#define CONFIG_SERIAL2
> -#endif
>
>  /* Provide the MACH_TYPE value the vendor kernel requires */
>  #define CONFIG_MACH_TYPE       3063
>
>
>
>
>
> cleanup_one_header() did a good job.
>
> Is there a particular case that the second path is needed?
>

Has moveconfig been updated to handle such cases in another way? I only
re-sent this because I found it while cleaning up some old branches. I
believe at the time I originally wrote it there were many more instances of
empty guards (most if which I submitted patches for).

I agree that if moveconfig already handles this there is no point in adding
a second pass (baring cases of double nesting).

There still may be benefit in running something as a one-off pass over all
files to remove existing empty guards but there's no need to store such a
script in the repository.

>