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

[Nikhil M Jain]

Hi Devarsh

On 17/04/23 11:09, Devarsh Thakkar wrote:
> 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 ?
I will remove it.

>>   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?
>>  
I will do a recheck.

>>   	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 ||
>>  
I will combine them.

>> @@ -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.
> 
Yes, it will be better if we just keep BMP check for bmp_display function.

> Regards
> Devarsh

Thanks,
Nikhil

>>   	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