[PATCH V7 14/14] common: Replace #ifdef and #if with if's

[Devarsh Thakkar]

Hi Nikhil,

Thanks for the patch.

On 10/04/23 12:58, Nikhil M Jain wrote:
> Remove #ifdef in header file to avoid multiple definitions while

multiple definitions doesnt seem to be right phrase as that will give
compilation error anyway.
> compilation. To improve code readability use if() rather than if's and
> '#ifdef's'.

Avoid using preprocessor compilation directives and instead use simple logical
expressions for better readability since compiler will anyway optimize
out the respective code block if condition is not satisfied.

> 
> Signed-off-by: Nikhil M Jain <n-jain1 at ti.com>
> ---
> V7(patch introduced):
> - Replace #ifdef and #if with if's.
> 
>  common/bmp.c     | 12 +++---------
>  common/splash.c  | 12 ++++++------
>  include/splash.h | 12 ------------
>  3 files changed, 9 insertions(+), 27 deletions(-)
> 
> diff --git a/common/bmp.c b/common/bmp.c
> index 51766b3c21..62b2d07d23 100644
> --- a/common/bmp.c
> +++ b/common/bmp.c
> @@ -33,7 +33,7 @@
>   * Returns NULL if decompression failed, or if the decompressed data
>   * didn't contain a valid BMP signature.

or decompression is not enabled in Kconfig.
>   */
> -#if CONFIG_IS_ENABLED(VIDEO_BMP_GZIP)
> +
nitpick, blank line.
>  struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp,
>  			     void **alloc_addr)
>  {
> @@ -41,6 +41,8 @@ struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp,
>  	unsigned long len;
>  	struct bmp_image *bmp;
>  
> +	if (!CONFIG_IS_ENABLED(VIDEO_BMP_GZIP))
> +		return NULL;
>  	/*
>  	 * Decompress bmp image
>  	 */
> @@ -77,14 +79,6 @@ struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp,
>  	*alloc_addr = dst;
>  	return bmp;
>  }
> -#else
> -struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp,
> -			     void **alloc_addr)
> -{
> -	return NULL;
> -}
> -#endif
> -
>  
>  #ifdef CONFIG_NEEDS_MANUAL_RELOC

Don't you want to remove this too and switch to if ?
>  void bmp_reloc(void) {
> diff --git a/common/splash.c b/common/splash.c
> index a4e68b7042..4c38a155d0 100644
> --- a/common/splash.c
> +++ b/common/splash.c
> @@ -79,7 +79,7 @@ static int splash_video_logo_load(void)
>  	}
>  
>  	memcpy((void *)bmp_load_addr, bmp_logo_bitmap,
> -	       ARRAY_SIZE(bmp_logo_bitmap));
> +		ARRAY_SIZE(bmp_logo_bitmap));

Is this intended? Does checkpatch work fine with that?
>  
>  	return 0;
>  }
> @@ -96,11 +96,12 @@ __weak int splash_screen_prepare(void)
>  	return splash_video_logo_load();
>  }
>  
> -#if CONFIG_IS_ENABLED(SPLASH_SCREEN_ALIGN)
>  void splash_get_pos(int *x, int *y)
>  {
>  	char *s = env_get("splashpos");
>  
> +	if (!CONFIG_IS_ENABLED(SPLASH_SCREEN_ALIGN))
> +		return;
>  	if (!s)
>  		return;

You can combine above both in a single statement using ||
>  
> @@ -117,7 +118,6 @@ void splash_get_pos(int *x, int *y)
>  			*y = simple_strtol(s + 1, NULL, 0);
>  	}
>  }
> -#endif /* CONFIG_SPLASH_SCREEN_ALIGN */
>  
>  #if CONFIG_IS_ENABLED(VIDEO) && !CONFIG_IS_ENABLED(HIDE_LOGO_VERSION)
>  

Don't you intend to remove above too and switch to if for all #ifdefs ?
> @@ -159,13 +159,14 @@ void splash_display_banner(void)
>   * Common function to show a splash image if env("splashimage") is set.
>   * For additional details please refer to doc/README.splashprepare.
>   */
> -#if CONFIG_IS_ENABLED(SPLASH_SCREEN) && CONFIG_IS_ENABLED(BMP)
> +
>  int splash_display(void)
>  {
>  	ulong addr;
>  	char *s;
>  	int x = 0, y = 0, ret;
> -
> +	if (!(CONFIG_IS_ENABLED(SPLASH_SCREEN) && CONFIG_IS_ENABLED(BMP)))
> +		return -ENOSYS;

I think BMP check should come under bmp specific code latter in the function.

Regards
Devarsh
>  	s = env_get("splashimage");
>  	if (!s)
>  		return -EINVAL;
> @@ -189,4 +190,3 @@ int splash_display(void)
>  end:
>  	return ret;
>  }
> -#endif
> diff --git a/include/splash.h b/include/splash.h
> index aa0c14024e..8f15adf9dd 100644
> --- a/include/splash.h
> +++ b/include/splash.h
> @@ -61,21 +61,9 @@ static inline int splash_source_load(struct splash_location *locations,
>  
>  int splash_screen_prepare(void);
>  
> -#if CONFIG_IS_ENABLED(SPLASH_SCREEN_ALIGN)
>  void splash_get_pos(int *x, int *y);
> -#else
> -static inline void splash_get_pos(int *x, int *y) { }
> -#endif
>  
> -#if CONFIG_IS_ENABLED(SPLASH_SCREEN) && CONFIG_IS_ENABLED(BMP)
>  int splash_display(void);
> -#else
> -static inline int splash_display(void)
> -{
> -	return -ENOSYS;
> -}
> -#endif
> -
>  #define BMP_ALIGN_CENTER	0x7FFF
>  
>  #endif
> \ No newline at end of file