Re: [U-Boot] [PATCH v3] moveconfig: add a second pass for empty #if/#endif blocks
On Thu, 31 Jan 2019 19:26 Masahiro Yamada On Wed, Jan 30, 2019 at 5:55 PM Masahiro Yamada > wrote: > > > > On Wed, Jan 30, 2019 at 4:23 PM Chris Packham > 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 > > > --- > > > 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 > 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 > > 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. > ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v3] moveconfig: add a second pass for empty #if/#endif blocks
On Wed, Jan 30, 2019 at 5:55 PM Masahiro Yamada wrote: > > On Wed, Jan 30, 2019 at 4:23 PM Chris Packham 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 > > --- > > 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. $ git show 9baa2bce -- include/configs/omap3_cairo.h commit 9baa2bce28901321d6f62399b5ebeb3fcb8e8a57 Author: Adam Ford 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 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? -- Best Regards Masahiro Yamada ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v3] moveconfig: add a second pass for empty #if/#endif blocks
On Wed, Jan 30, 2019 at 4:23 PM Chris Packham 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
> ---
> 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.
> Changes in v3:
> - drop rfc
>
> Changes in v2:
> - use re.compile
>
> tools/moveconfig.py | 27 +--
> 1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/tools/moveconfig.py b/tools/moveconfig.py
> index caa81ac2ed77..1a214c560546 100755
> --- a/tools/moveconfig.py
> +++ b/tools/moveconfig.py
> @@ -545,6 +545,28 @@ def confirm(options, prompt):
>
> return True
>
> +def cleanup_empty_blocks(header_path, options):
> +"""Clean up empty conditional blocks
> +
> +Arguments:
> + header_path: path to the cleaned file.
> + options: option flags.
> +"""
> +pattern = re.compile(r'^\s*#\s*if.*$\n^\s*#\s*endif.*$\n*', flags=re.M)
> +with open(header_path) as f:
> +data = f.read()
> +
> +new_data = pattern.sub('\n', data)
> +
> +show_diff(data.splitlines(True), new_data.splitlines(True), header_path,
> + options.color)
> +
> +if options.dry_run:
> +return
> +
> +with open(header_path, 'w') as f:
> +f.write(new_data)
> +
> def cleanup_one_header(header_path, patterns, options):
> """Clean regex-matched lines away from a file.
>
> @@ -626,8 +648,9 @@ def cleanup_headers(configs, options):
> continue
> for filename in filenames:
> if not fnmatch.fnmatch(filename, '*~'):
> -cleanup_one_header(os.path.join(dirpath, filename),
> - patterns, options)
> +header_path = os.path.join(dirpath, filename)
> +cleanup_one_header(header_path, patterns, options)
> +cleanup_empty_blocks(header_path, options)
>
> def cleanup_one_extra_option(defconfig_path, configs, options):
> """Delete config defines in CONFIG_SYS_EXTRA_OPTIONS in one defconfig
> file.
> --
> 2.20.1
>
> ___
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
--
Best Regards
Masahiro Yamada
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v3] moveconfig: add a second pass for empty #if/#endif blocks
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
---
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().
Changes in v3:
- drop rfc
Changes in v2:
- use re.compile
tools/moveconfig.py | 27 +--
1 file changed, 25 insertions(+), 2 deletions(-)
diff --git a/tools/moveconfig.py b/tools/moveconfig.py
index caa81ac2ed77..1a214c560546 100755
--- a/tools/moveconfig.py
+++ b/tools/moveconfig.py
@@ -545,6 +545,28 @@ def confirm(options, prompt):
return True
+def cleanup_empty_blocks(header_path, options):
+"""Clean up empty conditional blocks
+
+Arguments:
+ header_path: path to the cleaned file.
+ options: option flags.
+"""
+pattern = re.compile(r'^\s*#\s*if.*$\n^\s*#\s*endif.*$\n*', flags=re.M)
+with open(header_path) as f:
+data = f.read()
+
+new_data = pattern.sub('\n', data)
+
+show_diff(data.splitlines(True), new_data.splitlines(True), header_path,
+ options.color)
+
+if options.dry_run:
+return
+
+with open(header_path, 'w') as f:
+f.write(new_data)
+
def cleanup_one_header(header_path, patterns, options):
"""Clean regex-matched lines away from a file.
@@ -626,8 +648,9 @@ def cleanup_headers(configs, options):
continue
for filename in filenames:
if not fnmatch.fnmatch(filename, '*~'):
-cleanup_one_header(os.path.join(dirpath, filename),
- patterns, options)
+header_path = os.path.join(dirpath, filename)
+cleanup_one_header(header_path, patterns, options)
+cleanup_empty_blocks(header_path, options)
def cleanup_one_extra_option(defconfig_path, configs, options):
"""Delete config defines in CONFIG_SYS_EXTRA_OPTIONS in one defconfig file.
--
2.20.1
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
