diff mbox series

Fix Wsign-compare for int with ARRAY_SIZE() comparison

Message ID 20180116150902.20137-1-christian.storm@siemens.com
State Accepted
Headers show
Series Fix Wsign-compare for int with ARRAY_SIZE() comparison | expand

Commit Message

Christian Storm Jan. 16, 2018, 3:09 p.m. UTC
include/util.h's ARRAY_SIZE() uses a sizeof() calculation
which returns unsigned int. Hence fix Wsign-compare by
making the comparison variable unsigned int.

Signed-off-by: Christian Storm <christian.storm@siemens.com>
---
 corelib/installer.c        | 6 ++----
 corelib/stream_interface.c | 4 +---
 2 files changed, 3 insertions(+), 7 deletions(-)

Comments

Stefano Babic Jan. 16, 2018, 3:44 p.m. UTC | #1
Hi Christian,

On 16/01/2018 16:09, Christian Storm wrote:
> include/util.h's ARRAY_SIZE() uses a sizeof() calculation
> which returns unsigned int. Hence fix Wsign-compare by
> making the comparison variable unsigned int.
> 
> Signed-off-by: Christian Storm <christian.storm@siemens.com>
> ---
>  corelib/installer.c        | 6 ++----
>  corelib/stream_interface.c | 4 +---
>  2 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/corelib/installer.c b/corelib/installer.c
> index 4f58794..592923d 100644
> --- a/corelib/installer.c
> +++ b/corelib/installer.c
> @@ -423,11 +423,10 @@ static void remove_sw_file(char __attribute__ ((__unused__)) *fname)
>  static void cleaup_img_entry(struct img_type *img)
>  {
>  	char *fn;
> -	int i;
>  	const char *tmp[] = { get_tmpdirscripts(), get_tmpdir() };
>  
>  	if (img->fname[0]) {
> -		for (i = 0; i < ARRAY_SIZE(tmp); i++) {
> +		for (unsigned int i = 0; i < ARRAY_SIZE(tmp); i++) {
>  			if (asprintf(&fn, "%s%s", tmp[i], img->fname) == ENOMEM_ASPRINTF) {
>  				ERROR("Path too long: %s%s", tmp[i], img->fname);
>  			} else {
> @@ -444,7 +443,6 @@ void cleanup_files(struct swupdate_cfg *software) {
>  	struct img_type *img;
>  	struct hw_type *hw;
>  	const char* TMPDIR = get_tmpdir();
> -	int count;
>  	struct imglist *list[] = {&software->scripts, &software->bootscripts};
>  
>  	LIST_FOREACH(img, &software->images, next) {
> @@ -459,7 +457,7 @@ void cleanup_files(struct swupdate_cfg *software) {
>  		free(img);
>  	}
>  
> -	for (count = 0; count < ARRAY_SIZE(list); count++) {
> +	for (unsigned int count = 0; count < ARRAY_SIZE(list); count++) {

Right, thanks for fixing this.

>  		LIST_FOREACH(img, list[count], next) {
>  			cleaup_img_entry(img);
>  
> diff --git a/corelib/stream_interface.c b/corelib/stream_interface.c
> index 98b8ca9..a5fd4a1 100644
> --- a/corelib/stream_interface.c
> +++ b/corelib/stream_interface.c
> @@ -173,13 +173,11 @@ static int extract_files(int fd, struct swupdate_cfg *software)
>  				break;
>  			}
>  
> -			int i;
> -
>  			struct imglist *list[] = {&software->images,
>  						  &software->scripts,
>  						  &software->bootscripts};
>  
> -			for (i = 0; i < ARRAY_SIZE(list); i++) {
> +			for (unsigned int i = 0; i < ARRAY_SIZE(list); i++) {
>  				skip = check_if_required(list[i], &fdh,
>  						(list[i] == &software->images) ?
>  							&software->installed_sw_list : NULL,
> 

Reviewed-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic
Stefan Herbrechtsmeier Jan. 16, 2018, 7:01 p.m. UTC | #2
Hi Christian and Stefano,

Am 16.01.2018 um 16:44 schrieb Stefano Babic:
> On 16/01/2018 16:09, Christian Storm wrote:
>> include/util.h's ARRAY_SIZE() uses a sizeof() calculation
>> which returns unsigned int. Hence fix Wsign-compare by
>> making the comparison variable unsigned int.
>>
>> Signed-off-by: Christian Storm <christian.storm@siemens.com>
>> ---
>>   corelib/installer.c        | 6 ++----
>>   corelib/stream_interface.c | 4 +---
>>   2 files changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/corelib/installer.c b/corelib/installer.c
>> index 4f58794..592923d 100644
>> --- a/corelib/installer.c
>> +++ b/corelib/installer.c
>> @@ -423,11 +423,10 @@ static void remove_sw_file(char __attribute__ ((__unused__)) *fname)
>>   static void cleaup_img_entry(struct img_type *img)
>>   {
>>   	char *fn;
>> -	int i;
>>   	const char *tmp[] = { get_tmpdirscripts(), get_tmpdir() };
>>   
>>   	if (img->fname[0]) {
>> -		for (i = 0; i < ARRAY_SIZE(tmp); i++) {
>> +		for (unsigned int i = 0; i < ARRAY_SIZE(tmp); i++) {
>>   			if (asprintf(&fn, "%s%s", tmp[i], img->fname) == ENOMEM_ASPRINTF) {
>>   				ERROR("Path too long: %s%s", tmp[i], img->fname);
>>   			} else {
>> @@ -444,7 +443,6 @@ void cleanup_files(struct swupdate_cfg *software) {
>>   	struct img_type *img;
>>   	struct hw_type *hw;
>>   	const char* TMPDIR = get_tmpdir();
>> -	int count;
>>   	struct imglist *list[] = {&software->scripts, &software->bootscripts};
>>   
>>   	LIST_FOREACH(img, &software->images, next) {
>> @@ -459,7 +457,7 @@ void cleanup_files(struct swupdate_cfg *software) {
>>   		free(img);
>>   	}
>>   
>> -	for (count = 0; count < ARRAY_SIZE(list); count++) {
>> +	for (unsigned int count = 0; count < ARRAY_SIZE(list); count++) {
> Right, thanks for fixing this.
>
>>   		LIST_FOREACH(img, list[count], next) {
>>   			cleaup_img_entry(img);
>>   
>> diff --git a/corelib/stream_interface.c b/corelib/stream_interface.c
>> index 98b8ca9..a5fd4a1 100644
>> --- a/corelib/stream_interface.c
>> +++ b/corelib/stream_interface.c
>> @@ -173,13 +173,11 @@ static int extract_files(int fd, struct swupdate_cfg *software)
>>   				break;
>>   			}
>>   
>> -			int i;
>> -
>>   			struct imglist *list[] = {&software->images,
>>   						  &software->scripts,
>>   						  &software->bootscripts};
>>   
>> -			for (i = 0; i < ARRAY_SIZE(list); i++) {
>> +			for (unsigned int i = 0; i < ARRAY_SIZE(list); i++) {
>>   				skip = check_if_required(list[i], &fdh,
>>   						(list[i] == &software->images) ?
>>   							&software->installed_sw_list : NULL,
>>
> Reviewed-by: Stefano Babic <sbabic@denx.de>

This patch requires C99 standard support. Maybe we should set the c 
standard explicit or document it.

Best regards
   Stefan
Stefano Babic Jan. 16, 2018, 9:17 p.m. UTC | #3
Hi Stefan,

On 16/01/2018 20:01, Stefan Herbrechtsmeier wrote:
> Hi Christian and Stefano,

> 

> Am 16.01.2018 um 16:44 schrieb Stefano Babic:

>> On 16/01/2018 16:09, Christian Storm wrote:

>>> include/util.h's ARRAY_SIZE() uses a sizeof() calculation

>>> which returns unsigned int. Hence fix Wsign-compare by

>>> making the comparison variable unsigned int.

>>>

>>> Signed-off-by: Christian Storm <christian.storm@siemens.com>

>>> ---

>>>   corelib/installer.c        | 6 ++----

>>>   corelib/stream_interface.c | 4 +---

>>>   2 files changed, 3 insertions(+), 7 deletions(-)

>>>

>>> diff --git a/corelib/installer.c b/corelib/installer.c

>>> index 4f58794..592923d 100644

>>> --- a/corelib/installer.c

>>> +++ b/corelib/installer.c

>>> @@ -423,11 +423,10 @@ static void remove_sw_file(char __attribute__

>>> ((__unused__)) *fname)

>>>   static void cleaup_img_entry(struct img_type *img)

>>>   {

>>>       char *fn;

>>> -    int i;

>>>       const char *tmp[] = { get_tmpdirscripts(), get_tmpdir() };

>>>         if (img->fname[0]) {

>>> -        for (i = 0; i < ARRAY_SIZE(tmp); i++) {

>>> +        for (unsigned int i = 0; i < ARRAY_SIZE(tmp); i++) {

>>>               if (asprintf(&fn, "%s%s", tmp[i], img->fname) ==

>>> ENOMEM_ASPRINTF) {

>>>                   ERROR("Path too long: %s%s", tmp[i], img->fname);

>>>               } else {

>>> @@ -444,7 +443,6 @@ void cleanup_files(struct swupdate_cfg *software) {

>>>       struct img_type *img;

>>>       struct hw_type *hw;

>>>       const char* TMPDIR = get_tmpdir();

>>> -    int count;

>>>       struct imglist *list[] = {&software->scripts,

>>> &software->bootscripts};

>>>         LIST_FOREACH(img, &software->images, next) {

>>> @@ -459,7 +457,7 @@ void cleanup_files(struct swupdate_cfg *software) {

>>>           free(img);

>>>       }

>>>   -    for (count = 0; count < ARRAY_SIZE(list); count++) {

>>> +    for (unsigned int count = 0; count < ARRAY_SIZE(list); count++) {

>> Right, thanks for fixing this.

>>

>>>           LIST_FOREACH(img, list[count], next) {

>>>               cleaup_img_entry(img);

>>>   diff --git a/corelib/stream_interface.c b/corelib/stream_interface.c

>>> index 98b8ca9..a5fd4a1 100644

>>> --- a/corelib/stream_interface.c

>>> +++ b/corelib/stream_interface.c

>>> @@ -173,13 +173,11 @@ static int extract_files(int fd, struct

>>> swupdate_cfg *software)

>>>                   break;

>>>               }

>>>   -            int i;

>>> -

>>>               struct imglist *list[] = {&software->images,

>>>                             &software->scripts,

>>>                             &software->bootscripts};

>>>   -            for (i = 0; i < ARRAY_SIZE(list); i++) {

>>> +            for (unsigned int i = 0; i < ARRAY_SIZE(list); i++) {

>>>                   skip = check_if_required(list[i], &fdh,

>>>                           (list[i] == &software->images) ?

>>>                               &software->installed_sw_list : NULL,

>>>

>> Reviewed-by: Stefano Babic <sbabic@denx.de>

> 

> This patch requires C99 standard support.


Well, not only this patch.....

> Maybe we should set the c

> standard explicit or document it.


Something about compiler requirement is missing in documentation. I vote
to add this to documentation.

Best regards,
Stefano

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
=====================================================================
Christian Storm Jan. 17, 2018, 7:56 a.m. UTC | #4
Hi,

> > This patch requires C99 standard support.
> 
> Well, not only this patch.....
> 
> > Maybe we should set the c
> > standard explicit or document it.

Well, it's actually set, explicitly. See Makefile.flags:10:
  KBUILD_CPPFLAGS += $(call cc-option,-std=gnu99,)
OK, the gnu "dialect" but nonetheless C99.

> Something about compiler requirement is missing in documentation.
> I vote to add this to documentation.

Hm, what compiler nowadays doesn't support that standard coming
from the last millennium, nearly twenty years after its invention? :)
That said, I'm not against documenting this at, e.g.,
source/swupdate.rst:201


Kind regards,
   Christian
Stefano Babic Jan. 17, 2018, 8:14 a.m. UTC | #5
On 17/01/2018 08:56, Christian Storm wrote:
> Hi,
> 
>>> This patch requires C99 standard support.
>>
>> Well, not only this patch.....
>>
>>> Maybe we should set the c
>>> standard explicit or document it.
> 
> Well, it's actually set, explicitly. See Makefile.flags:10:
>   KBUILD_CPPFLAGS += $(call cc-option,-std=gnu99,)
> OK, the gnu "dialect" but nonetheless C99.
> 
>> Something about compiler requirement is missing in documentation.
>> I vote to add this to documentation.
> 
> Hm, what compiler nowadays doesn't support that standard coming
> from the last millennium, nearly twenty years after its invention? :)

Indeed. I thought when I started the project if I had to stick with
plain K&R language, maybe if SWUpdate can be ported to a lower
(=microcontroller) device. But all compilers support nowadays the new
standard.

> That said, I'm not against documenting this at, e.g.,
> source/swupdate.rst:201
> 

Best regards,
Stefano
diff mbox series

Patch

diff --git a/corelib/installer.c b/corelib/installer.c
index 4f58794..592923d 100644
--- a/corelib/installer.c
+++ b/corelib/installer.c
@@ -423,11 +423,10 @@  static void remove_sw_file(char __attribute__ ((__unused__)) *fname)
 static void cleaup_img_entry(struct img_type *img)
 {
 	char *fn;
-	int i;
 	const char *tmp[] = { get_tmpdirscripts(), get_tmpdir() };
 
 	if (img->fname[0]) {
-		for (i = 0; i < ARRAY_SIZE(tmp); i++) {
+		for (unsigned int i = 0; i < ARRAY_SIZE(tmp); i++) {
 			if (asprintf(&fn, "%s%s", tmp[i], img->fname) == ENOMEM_ASPRINTF) {
 				ERROR("Path too long: %s%s", tmp[i], img->fname);
 			} else {
@@ -444,7 +443,6 @@  void cleanup_files(struct swupdate_cfg *software) {
 	struct img_type *img;
 	struct hw_type *hw;
 	const char* TMPDIR = get_tmpdir();
-	int count;
 	struct imglist *list[] = {&software->scripts, &software->bootscripts};
 
 	LIST_FOREACH(img, &software->images, next) {
@@ -459,7 +457,7 @@  void cleanup_files(struct swupdate_cfg *software) {
 		free(img);
 	}
 
-	for (count = 0; count < ARRAY_SIZE(list); count++) {
+	for (unsigned int count = 0; count < ARRAY_SIZE(list); count++) {
 		LIST_FOREACH(img, list[count], next) {
 			cleaup_img_entry(img);
 
diff --git a/corelib/stream_interface.c b/corelib/stream_interface.c
index 98b8ca9..a5fd4a1 100644
--- a/corelib/stream_interface.c
+++ b/corelib/stream_interface.c
@@ -173,13 +173,11 @@  static int extract_files(int fd, struct swupdate_cfg *software)
 				break;
 			}
 
-			int i;
-
 			struct imglist *list[] = {&software->images,
 						  &software->scripts,
 						  &software->bootscripts};
 
-			for (i = 0; i < ARRAY_SIZE(list); i++) {
+			for (unsigned int i = 0; i < ARRAY_SIZE(list); i++) {
 				skip = check_if_required(list[i], &fdh,
 						(list[i] == &software->images) ?
 							&software->installed_sw_list : NULL,