[PATCH libdrm 1/1] tests/exynos: Fix warnings

[PATCH libdrm 1/1] tests/exynos: Fix warnings

[Jan Vesely]

Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu>
---
 tests/exynos/exynos_fimg2d_test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/exynos/exynos_fimg2d_test.c b/tests/exynos/exynos_fimg2d_test.c
index e7d9b72..dfb34ac 100644
--- a/tests/exynos/exynos_fimg2d_test.c
+++ b/tests/exynos/exynos_fimg2d_test.c
@@ -183,7 +183,7 @@ static struct exynos_bo *exynos_create_buffer(struct exynos_device *dev,
 
 /* Allocate buffer and fill it with checkerboard pattern, where the tiles *
  * have a random color. The caller has to free the buffer.                */
-void *create_checkerboard_pattern(unsigned int num_tiles_x,
+static void *create_checkerboard_pattern(unsigned int num_tiles_x,
 						unsigned int num_tiles_y, unsigned int tile_size)
 {
 	unsigned int *buf;
@@ -573,6 +573,7 @@ static int g2d_checkerboard_test(struct exynos_device *dev,
 		src_img.user_ptr[0].userptr = (unsigned long)checkerboard;
 		src_img.user_ptr[0].size = img_w * img_h * 4;
 		break;
+	case G2D_IMGBUF_COLOR:
 	default:
 		ret = -EFAULT;
 		goto fail;
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 1/1] tests/exynos: Fix warnings

[Tobias Jakobi]

Hello Jan,

Jan Vesely wrote:
> Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu>
> ---
>  tests/exynos/exynos_fimg2d_test.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/exynos/exynos_fimg2d_test.c b/tests/exynos/exynos_fimg2d_test.c
> index e7d9b72..dfb34ac 100644
> --- a/tests/exynos/exynos_fimg2d_test.c
> +++ b/tests/exynos/exynos_fimg2d_test.c
> @@ -183,7 +183,7 @@ static struct exynos_bo *exynos_create_buffer(struct exynos_device *dev,
>  
>  /* Allocate buffer and fill it with checkerboard pattern, where the tiles *
>   * have a random color. The caller has to free the buffer.                */
> -void *create_checkerboard_pattern(unsigned int num_tiles_x,
> +static void *create_checkerboard_pattern(unsigned int num_tiles_x,
>  						unsigned int num_tiles_y, unsigned int tile_size)
>  {
>  	unsigned int *buf;
Good catch with the missing static!


> @@ -573,6 +573,7 @@ static int g2d_checkerboard_test(struct exynos_device *dev,
>  		src_img.user_ptr[0].userptr = (unsigned long)checkerboard;
>  		src_img.user_ptr[0].size = img_w * img_h * 4;
>  		break;
> +	case G2D_IMGBUF_COLOR:
>  	default:
>  		ret = -EFAULT;
>  		goto fail;
> 
Hmm, I don't see the reason why this label should be added to the switch
statement?

With best wishes,
Tobias

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 1/1] tests/exynos: Fix warnings

[Jan Vesely]

[-- Attachment #1.1: Type: text/plain, Size: 1947 bytes --]

On Wed, 2015-03-18 at 20:23 +0100, Tobias Jakobi wrote:
> Hello Jan,
> 
> Jan Vesely wrote:
> > Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu>
> > ---
> >  tests/exynos/exynos_fimg2d_test.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/exynos/exynos_fimg2d_test.c b/tests/exynos/exynos_fimg2d_test.c
> > index e7d9b72..dfb34ac 100644
> > --- a/tests/exynos/exynos_fimg2d_test.c
> > +++ b/tests/exynos/exynos_fimg2d_test.c
> > @@ -183,7 +183,7 @@ static struct exynos_bo *exynos_create_buffer(struct exynos_device *dev,
> >  
> >  /* Allocate buffer and fill it with checkerboard pattern, where the tiles *
> >   * have a random color. The caller has to free the buffer.                */
> > -void *create_checkerboard_pattern(unsigned int num_tiles_x,
> > +static void *create_checkerboard_pattern(unsigned int num_tiles_x,
> >  						unsigned int num_tiles_y, unsigned int tile_size)
> >  {
> >  	unsigned int *buf;
> Good catch with the missing static!
> 
> 
> > @@ -573,6 +573,7 @@ static int g2d_checkerboard_test(struct exynos_device *dev,
> >  		src_img.user_ptr[0].userptr = (unsigned long)checkerboard;
> >  		src_img.user_ptr[0].size = img_w * img_h * 4;
> >  		break;
> > +	case G2D_IMGBUF_COLOR:
> >  	default:
> >  		ret = -EFAULT;
> >  		goto fail;
> > 
> Hmm, I don't see the reason why this label should be added to the switch
> statement?

This is mostly to appease the compiler. -Wswitch-enum warns about
missing enumeration cases even if default is present. On the other hand
it also warns against branches that use values outside of the
enumeration (which might be useful).

Emil, any thoughts about possibly dropping this warning? I think more
people will scratch their heads if unused enumeration values need to be
added to default branch.


jan

> 
> With best wishes,
> Tobias
> 

-- 
Jan Vesely <jan.vesely@rutgers.edu>

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 1/1] tests/exynos: Fix warnings

[Tobias Jakobi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jan Vesely wrote:
>> 
>>> @@ -573,6 +573,7 @@ static int g2d_checkerboard_test(struct
>>> exynos_device *dev, src_img.user_ptr[0].userptr = (unsigned
>>> long)checkerboard; src_img.user_ptr[0].size = img_w * img_h *
>>> 4; break; +	case G2D_IMGBUF_COLOR: default: ret = -EFAULT; goto
>>> fail;
>>> 
>> Hmm, I don't see the reason why this label should be added to the
>> switch statement?
> 
> This is mostly to appease the compiler. -Wswitch-enum warns about 
> missing enumeration cases even if default is present. On the other
> hand it also warns against branches that use values outside of the 
> enumeration (which might be useful).
Ah, that makes sense. I guess adding the label is a good thing then!

With best wishes,
Tobias

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iEYEARECAAYFAlUJ3UYACgkQ6bCODwikBXd6xgCdG4sZERoY6KalhPn6AhyoxfbO
8EQAnRrCZrPXN/fjqwZT4X6a/7wLofeJ
=C/FO
-----END PGP SIGNATURE-----
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 1/1] tests/exynos: Fix warnings

[Emil Velikov]

On 18/03/15 19:41, Jan Vesely wrote:
> On Wed, 2015-03-18 at 20:23 +0100, Tobias Jakobi wrote:
>> Hello Jan,
>>
>> Jan Vesely wrote:
>>> Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu>
>>> ---
>>>  tests/exynos/exynos_fimg2d_test.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/exynos/exynos_fimg2d_test.c b/tests/exynos/exynos_fimg2d_test.c
>>> index e7d9b72..dfb34ac 100644
>>> --- a/tests/exynos/exynos_fimg2d_test.c
>>> +++ b/tests/exynos/exynos_fimg2d_test.c
>>> @@ -183,7 +183,7 @@ static struct exynos_bo *exynos_create_buffer(struct exynos_device *dev,
>>>  
>>>  /* Allocate buffer and fill it with checkerboard pattern, where the tiles *
>>>   * have a random color. The caller has to free the buffer.                */
>>> -void *create_checkerboard_pattern(unsigned int num_tiles_x,
>>> +static void *create_checkerboard_pattern(unsigned int num_tiles_x,
>>>  						unsigned int num_tiles_y, unsigned int tile_size)
>>>  {
>>>  	unsigned int *buf;
>> Good catch with the missing static!
>>
>>
>>> @@ -573,6 +573,7 @@ static int g2d_checkerboard_test(struct exynos_device *dev,
>>>  		src_img.user_ptr[0].userptr = (unsigned long)checkerboard;
>>>  		src_img.user_ptr[0].size = img_w * img_h * 4;
>>>  		break;
>>> +	case G2D_IMGBUF_COLOR:
>>>  	default:
>>>  		ret = -EFAULT;
>>>  		goto fail;
>>>
>> Hmm, I don't see the reason why this label should be added to the switch
>> statement?
> 
> This is mostly to appease the compiler. -Wswitch-enum warns about
> missing enumeration cases even if default is present. On the other hand
> it also warns against branches that use values outside of the
> enumeration (which might be useful).
> 
> Emil, any thoughts about possibly dropping this warning? I think more
> people will scratch their heads if unused enumeration values need to be
> added to default branch.
> 
Fwiw mesa (and other projects) have addopted the approach of explicitly
handling each enum, and omitting the default statement.

Although considering that 1) this is a seldom run test and 2) things
have been as is (G2D_IMGBUF_COLOR implicitly handled by default) since
day 1, I'd say that either way is fine. Ideally Exynos people will cast
their vote, although I'm suspecting that this is not on their list.

Cheers,
Emil

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 1/1] tests/exynos: Fix warnings

[Jan Vesely]

[-- Attachment #1.1: Type: text/plain, Size: 1670 bytes --]

On Wed, 2015-03-18 at 20:23 +0100, Tobias Jakobi wrote:
> Hello Jan,
> 
> Jan Vesely wrote:
> > Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu>
> > ---
> >  tests/exynos/exynos_fimg2d_test.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/exynos/exynos_fimg2d_test.c b/tests/exynos/exynos_fimg2d_test.c
> > index e7d9b72..dfb34ac 100644
> > --- a/tests/exynos/exynos_fimg2d_test.c
> > +++ b/tests/exynos/exynos_fimg2d_test.c
> > @@ -183,7 +183,7 @@ static struct exynos_bo *exynos_create_buffer(struct exynos_device *dev,
> >  
> >  /* Allocate buffer and fill it with checkerboard pattern, where the tiles *
> >   * have a random color. The caller has to free the buffer.                */
> > -void *create_checkerboard_pattern(unsigned int num_tiles_x,
> > +static void *create_checkerboard_pattern(unsigned int num_tiles_x,
> >  						unsigned int num_tiles_y, unsigned int tile_size)
> >  {
> >  	unsigned int *buf;
> Good catch with the missing static!

May I consider this a R-b for this change?
I have moved the switch-enum fix to a separate patch (that covers all
switch-enum warnings)

jan

> 
> 
> > @@ -573,6 +573,7 @@ static int g2d_checkerboard_test(struct exynos_device *dev,
> >  		src_img.user_ptr[0].userptr = (unsigned long)checkerboard;
> >  		src_img.user_ptr[0].size = img_w * img_h * 4;
> >  		break;
> > +	case G2D_IMGBUF_COLOR:
> >  	default:
> >  		ret = -EFAULT;
> >  		goto fail;
> > 
> Hmm, I don't see the reason why this label should be added to the switch
> statement?
> 
> With best wishes,
> Tobias
> 

-- 
Jan Vesely <jan.vesely@rutgers.edu>

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 1/1] tests/exynos: Fix warnings

[Tobias Jakobi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hello Jan,

Jan Vesely wrote:
> May I consider this a R-b for this change? I have moved the
> switch-enum fix to a separate patch (that covers all switch-enum
> warnings)
sure, that's fine with me.

With best wishes,
Tobias

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iEYEARECAAYFAlUMm7gACgkQ6bCODwikBXdquwCg5w2mnXFdMIdmyZ1V4P+1bK21
kAMAnRMSeU9QFLIRF+F4jRWB2aq9wmtK
=gdXV
-----END PGP SIGNATURE-----
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel