diff mbox

sna: Initialize variable 'iter' to silence clang

Message ID 1361888117-19708-1-git-send-email-sedat.dilek@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sedat Dilek Feb. 26, 2013, 2:15 p.m. UTC 
Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com>
---
 src/sna/sna_damage.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Paul Menzel Feb. 28, 2013, 8:45 a.m. UTC | #1 
Dear Sedat,


thank you for testing stuff with Clang!


Am Dienstag, den 26.02.2013, 15:15 +0100 schrieb Sedat Dilek:

Please always paste the error/warning messages so reviewers see exactly
what it has find.

> Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com>
> ---
>  src/sna/sna_damage.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/sna/sna_damage.c b/src/sna/sna_damage.c
> index ab693af..53ed635 100644
> --- a/src/sna/sna_damage.c
> +++ b/src/sna/sna_damage.c
> @@ -410,7 +410,7 @@ static void __sna_damage_reduce(struct sna_damage *damage)
>  	int n, nboxes;
>  	BoxPtr boxes, free_boxes = NULL;
>  	pixman_region16_t *region = &damage->region;
> -	struct sna_damage_box *iter;
> +	struct sna_damage_box *iter = NULL;

Looking at the macro in `src/intel_list.h`

        /**
         * Loop through the list given by head and set pos to struct in the list.
         *
         * Example:
         * struct foo *iterator;
         * list_for_each_entry(iterator, &bar->list_of_foos, entry) {
         *      [modify iterator]
         * }
         *
         * This macro is not safe for node deletion. Use list_for_each_entry_safe
         * instead.
         *
         * @param pos Iterator variable of the type of the list elements.
         * @param head List head
         * @param member Member name of the struct list in the list elements.
         *
         */
        #define list_for_each_entry(pos, head, member)				\
            for (pos = __container_of((head)->next, pos, member);		\
        	 &pos->member != (head);					\
        	 pos = __container_of(pos->member.next, pos, member))

the iterator is definitely initialized in the for loop.

        pos = __container_of((head)->next

>  	assert(damage->mode != DAMAGE_ALL);
>  	assert(damage->dirty);
> @@ -1671,7 +1671,7 @@ void _sna_damage_debug_get_region(struct sna_damage *damage, RegionRec *r)
>  {
>  	int n, nboxes;
>  	BoxPtr boxes;
> -	struct sna_damage_box *iter;
> +	struct sna_damage_box *iter = NULL;
>  
>  	RegionCopy(r, &damage->region);
>  	if (!damage->dirty)

So in my opinion this patch should not be applied as it would prevent
the compiler to warn next time when there is really a code change where
this variable might be uninitialized.

By the way, does `gcc` warn about this?


Thanks,

Paul
Paul Menzel Feb. 28, 2013, 11:40 a.m. UTC | #2 
Dear Sedat,


Am Donnerstag, den 28.02.2013, 09:45 +0100 schrieb Paul Menzel:

> thank you for testing stuff with Clang!
> 
> 
> Am Dienstag, den 26.02.2013, 15:15 +0100 schrieb Sedat Dilek:
> 
> Please always paste the error/warning messages so reviewers see exactly
> what it has find.
> 
> > Signed-off-by: Sedat Dilek <sedat.dilek@gmail.com>
> > ---
> >  src/sna/sna_damage.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/sna/sna_damage.c b/src/sna/sna_damage.c
> > index ab693af..53ed635 100644
> > --- a/src/sna/sna_damage.c
> > +++ b/src/sna/sna_damage.c
> > @@ -410,7 +410,7 @@ static void __sna_damage_reduce(struct sna_damage *damage)
> >  	int n, nboxes;
> >  	BoxPtr boxes, free_boxes = NULL;
> >  	pixman_region16_t *region = &damage->region;
> > -	struct sna_damage_box *iter;
> > +	struct sna_damage_box *iter = NULL;
> 
> Looking at the macro in `src/intel_list.h`
> 
>         /**
>          * Loop through the list given by head and set pos to struct in the list.
>          *
>          * Example:
>          * struct foo *iterator;
>          * list_for_each_entry(iterator, &bar->list_of_foos, entry) {
>          *      [modify iterator]
>          * }
>          *
>          * This macro is not safe for node deletion. Use list_for_each_entry_safe
>          * instead.
>          *
>          * @param pos Iterator variable of the type of the list elements.
>          * @param head List head
>          * @param member Member name of the struct list in the list elements.
>          *
>          */
>         #define list_for_each_entry(pos, head, member)				\
>             for (pos = __container_of((head)->next, pos, member);		\
>         	 &pos->member != (head);					\
>         	 pos = __container_of(pos->member.next, pos, member))
> 
> the iterator is definitely initialized in the for loop.
> 
>         pos = __container_of((head)->next
> 
> >  	assert(damage->mode != DAMAGE_ALL);
> >  	assert(damage->dirty);
> > @@ -1671,7 +1671,7 @@ void _sna_damage_debug_get_region(struct sna_damage *damage, RegionRec *r)
> >  {
> >  	int n, nboxes;
> >  	BoxPtr boxes;
> > -	struct sna_damage_box *iter;
> > +	struct sna_damage_box *iter = NULL;
> >  
> >  	RegionCopy(r, &damage->region);
> >  	if (!damage->dirty)
> 
> So in my opinion this patch should not be applied as it would prevent
> the compiler to warn next time when there is really a code change where
> this variable might be uninitialized.
> 
> By the way, does `gcc` warn about this?

Using GCC I only see the following warnings in `sna_damage.c`.

        $ gcc -v
        Using built-in specs.
        COLLECT_GCC=/usr/bin/gcc-4.7.real
        COLLECT_LTO_WRAPPER=/usr/lib/gcc/i486-linux-gnu/4.7/lto-wrapper
        Target: i486-linux-gnu
        Configured with: ../src/configure -v --with-pkgversion='Debian 4.7.2-15' --with-bugurl=file:///usr/share/doc/gcc-4.7/README.Bugs --enable-languages=c,c++,go,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-4.7 --enable-shared --enable-linker-build-id --with-system-zlib --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.7 --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --enable-gnu-unique-object --enable-plugin --enable-objc-gc --enable-targets=all --enable-multiarch --with-arch-32=i586 --with-multilib-list=m32,m64,mx32 --with-tune=generic --enable-checking=release --build=i486-linux-gnu --host=i486-linux-gnu --target=i486-linux-gnu
        Thread model: posix
        gcc version 4.7.2 (Debian 4.7.2-15)
        $ ./autogen.sh # I’d prefer it did not run ./configure
        $ make
        […]
        sna_damage.c: In function '__sna_damage_add_box':
        sna_damage.c:601:11: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual]
        sna_damage.c: In function '__sna_damage_contains_box':
        sna_damage.c:1292:58: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual]
        sna_damage.c:1305:59: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual]
        sna_damage.c: In function '_sna_damage_contains_box__no_reduce':
        sna_damage.c:1346:42: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual]
        sna_damage.c:1347:7: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual]
        […]


Thanks,

Paul
diff mbox

Patch

diff --git a/src/sna/sna_damage.c b/src/sna/sna_damage.c
index ab693af..53ed635 100644
--- a/src/sna/sna_damage.c
+++ b/src/sna/sna_damage.c
@@ -410,7 +410,7 @@  static void __sna_damage_reduce(struct sna_damage *damage)
 	int n, nboxes;
 	BoxPtr boxes, free_boxes = NULL;
 	pixman_region16_t *region = &damage->region;
-	struct sna_damage_box *iter;
+	struct sna_damage_box *iter = NULL;
 
 	assert(damage->mode != DAMAGE_ALL);
 	assert(damage->dirty);
@@ -1671,7 +1671,7 @@  void _sna_damage_debug_get_region(struct sna_damage *damage, RegionRec *r)
 {
 	int n, nboxes;
 	BoxPtr boxes;
-	struct sna_damage_box *iter;
+	struct sna_damage_box *iter = NULL;
 
 	RegionCopy(r, &damage->region);
 	if (!damage->dirty)