[Spice-devel,xf86-video-qxl,04/13] qxl_driver: cleanup: fix const cast warnings

Submitted by Alon Levy on April 9, 2012, 5:33 p.m.

Details

Message ID 1333992799-14302-5-git-send-email-alevy@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Alon Levy April 9, 2012, 5:33 p.m.
---
 src/qxl_driver.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/qxl_driver.c b/src/qxl_driver.c
index 5c826f3..82a6ff1 100644
--- a/src/qxl_driver.c
+++ b/src/qxl_driver.c
@@ -1617,6 +1617,8 @@  static PciChipsets qxlPciChips[] =
 #endif
 #endif /* !XSPICE */
 
+static char qxl_driver_name[] = QXL_DRIVER_NAME;
+
 static void
 qxl_identify(int flags)
 {
@@ -1629,7 +1631,7 @@  static void
 qxl_init_scrn(ScrnInfoPtr pScrn)
 {
     pScrn->driverVersion    = 0;
-    pScrn->driverName	    = pScrn->name = QXL_DRIVER_NAME;
+    pScrn->driverName	    = pScrn->name = qxl_driver_name;
     pScrn->PreInit	    = qxl_pre_init;
     pScrn->ScreenInit	    = qxl_screen_init;
     pScrn->SwitchMode	    = qxl_switch_mode;
@@ -1741,7 +1743,7 @@  qxl_pci_probe(DriverPtr drv, int entity, struct pci_device *dev, intptr_t match)
 
 static DriverRec qxl_driver = {
     0,
-    QXL_DRIVER_NAME,
+    qxl_driver_name,
     qxl_identify,
     qxl_probe,
     qxl_available_options,

Comments

On Mon, Apr 09, 2012 at 08:33:10PM +0300, Alon Levy wrote:
> ---
>  src/qxl_driver.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qxl_driver.c b/src/qxl_driver.c
> index 5c826f3..82a6ff1 100644
> --- a/src/qxl_driver.c
> +++ b/src/qxl_driver.c
> @@ -1617,6 +1617,8 @@ static PciChipsets qxlPciChips[] =
>  #endif
>  #endif /* !XSPICE */
>  
> +static char qxl_driver_name[] = QXL_DRIVER_NAME;
> +
>  static void
>  qxl_identify(int flags)
>  {
> @@ -1629,7 +1631,7 @@ static void
>  qxl_init_scrn(ScrnInfoPtr pScrn)
>  {
>      pScrn->driverVersion    = 0;
> -    pScrn->driverName	    = pScrn->name = QXL_DRIVER_NAME;
> +    pScrn->driverName	    = pScrn->name = qxl_driver_name;
>      pScrn->PreInit	    = qxl_pre_init;
>      pScrn->ScreenInit	    = qxl_screen_init;
>      pScrn->SwitchMode	    = qxl_switch_mode;
> @@ -1741,7 +1743,7 @@ qxl_pci_probe(DriverPtr drv, int entity, struct pci_device *dev, intptr_t match)
>  
>  static DriverRec qxl_driver = {
>      0,
> -    QXL_DRIVER_NAME,
> +    qxl_driver_name,
>      qxl_identify,
>      qxl_probe,
>      qxl_available_options,

Will xorg attempt to modify driverName? If not, I'd just add (char *) casts
instead of creating a copy of QXL_DRIVER_NAME. Or is it customary to do
things this way in X drivers?

Christophe
On Tue, Apr 10, 2012 at 12:18:11PM +0200, Christophe Fergeau wrote:
> On Mon, Apr 09, 2012 at 08:33:10PM +0300, Alon Levy wrote:
> > ---
> >  src/qxl_driver.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/qxl_driver.c b/src/qxl_driver.c
> > index 5c826f3..82a6ff1 100644
> > --- a/src/qxl_driver.c
> > +++ b/src/qxl_driver.c
> > @@ -1617,6 +1617,8 @@ static PciChipsets qxlPciChips[] =
> >  #endif
> >  #endif /* !XSPICE */
> >  
> > +static char qxl_driver_name[] = QXL_DRIVER_NAME;
> > +
> >  static void
> >  qxl_identify(int flags)
> >  {
> > @@ -1629,7 +1631,7 @@ static void
> >  qxl_init_scrn(ScrnInfoPtr pScrn)
> >  {
> >      pScrn->driverVersion    = 0;
> > -    pScrn->driverName	    = pScrn->name = QXL_DRIVER_NAME;
> > +    pScrn->driverName	    = pScrn->name = qxl_driver_name;
> >      pScrn->PreInit	    = qxl_pre_init;
> >      pScrn->ScreenInit	    = qxl_screen_init;
> >      pScrn->SwitchMode	    = qxl_switch_mode;
> > @@ -1741,7 +1743,7 @@ qxl_pci_probe(DriverPtr drv, int entity, struct pci_device *dev, intptr_t match)
> >  
> >  static DriverRec qxl_driver = {
> >      0,
> > -    QXL_DRIVER_NAME,
> > +    qxl_driver_name,
> >      qxl_identify,
> >      qxl_probe,
> >      qxl_available_options,
> 
> Will xorg attempt to modify driverName? If not, I'd just add (char *) casts
> instead of creating a copy of QXL_DRIVER_NAME. Or is it customary to do
> things this way in X drivers?

No reason to assume it would change it, this is purely to remove a
warning - I know I'm doing it the wrong way, but (char *) doesn't fix
the warning. I could remove the -W flag used, but I don't like that
solution. It can't be stack allocated since it's used later, so I think
this is actually right (changing my mind from earlier this paragraph).

> 
> Christophe
On Tue, Apr 10, 2012 at 02:59:02PM +0300, Alon Levy wrote:
> On Tue, Apr 10, 2012 at 12:18:11PM +0200, Christophe Fergeau wrote:
> > On Mon, Apr 09, 2012 at 08:33:10PM +0300, Alon Levy wrote:
> > > ---
> > >  src/qxl_driver.c |    6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/qxl_driver.c b/src/qxl_driver.c
> > > index 5c826f3..82a6ff1 100644
> > > --- a/src/qxl_driver.c
> > > +++ b/src/qxl_driver.c
> > > @@ -1617,6 +1617,8 @@ static PciChipsets qxlPciChips[] =
> > >  #endif
> > >  #endif /* !XSPICE */
> > >  
> > > +static char qxl_driver_name[] = QXL_DRIVER_NAME;
> > > +
> > >  static void
> > >  qxl_identify(int flags)
> > >  {
> > > @@ -1629,7 +1631,7 @@ static void
> > >  qxl_init_scrn(ScrnInfoPtr pScrn)
> > >  {
> > >      pScrn->driverVersion    = 0;
> > > -    pScrn->driverName	    = pScrn->name = QXL_DRIVER_NAME;
> > > +    pScrn->driverName	    = pScrn->name = qxl_driver_name;
> > >      pScrn->PreInit	    = qxl_pre_init;
> > >      pScrn->ScreenInit	    = qxl_screen_init;
> > >      pScrn->SwitchMode	    = qxl_switch_mode;
> > > @@ -1741,7 +1743,7 @@ qxl_pci_probe(DriverPtr drv, int entity, struct pci_device *dev, intptr_t match)
> > >  
> > >  static DriverRec qxl_driver = {
> > >      0,
> > > -    QXL_DRIVER_NAME,
> > > +    qxl_driver_name,
> > >      qxl_identify,
> > >      qxl_probe,
> > >      qxl_available_options,
> > 
> > Will xorg attempt to modify driverName? If not, I'd just add (char *) casts
> > instead of creating a copy of QXL_DRIVER_NAME. Or is it customary to do
> > things this way in X drivers?
> 
> No reason to assume it would change it, this is purely to remove a
> warning - I know I'm doing it the wrong way, but (char *) doesn't fix
> the warning. I could remove the -W flag used, but I don't like that
> solution.

Ah, what is the warning exactly?

Christophe

> It can't be stack allocated since it's used later, so I think
> this is actually right (changing my mind from earlier this paragraph).
> 
> > 
> > Christophe
> 
>
On Tue, Apr 10, 2012 at 02:06:25PM +0200, Christophe Fergeau wrote:
> On Tue, Apr 10, 2012 at 02:59:02PM +0300, Alon Levy wrote:
> > On Tue, Apr 10, 2012 at 12:18:11PM +0200, Christophe Fergeau wrote:
> > > On Mon, Apr 09, 2012 at 08:33:10PM +0300, Alon Levy wrote:
> > > > ---
> > > >  src/qxl_driver.c |    6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/src/qxl_driver.c b/src/qxl_driver.c
> > > > index 5c826f3..82a6ff1 100644
> > > > --- a/src/qxl_driver.c
> > > > +++ b/src/qxl_driver.c
> > > > @@ -1617,6 +1617,8 @@ static PciChipsets qxlPciChips[] =
> > > >  #endif
> > > >  #endif /* !XSPICE */
> > > >  
> > > > +static char qxl_driver_name[] = QXL_DRIVER_NAME;
> > > > +
> > > >  static void
> > > >  qxl_identify(int flags)
> > > >  {
> > > > @@ -1629,7 +1631,7 @@ static void
> > > >  qxl_init_scrn(ScrnInfoPtr pScrn)
> > > >  {
> > > >      pScrn->driverVersion    = 0;
> > > > -    pScrn->driverName	    = pScrn->name = QXL_DRIVER_NAME;
> > > > +    pScrn->driverName	    = pScrn->name = qxl_driver_name;
> > > >      pScrn->PreInit	    = qxl_pre_init;
> > > >      pScrn->ScreenInit	    = qxl_screen_init;
> > > >      pScrn->SwitchMode	    = qxl_switch_mode;
> > > > @@ -1741,7 +1743,7 @@ qxl_pci_probe(DriverPtr drv, int entity, struct pci_device *dev, intptr_t match)
> > > >  
> > > >  static DriverRec qxl_driver = {
> > > >      0,
> > > > -    QXL_DRIVER_NAME,
> > > > +    qxl_driver_name,
> > > >      qxl_identify,
> > > >      qxl_probe,
> > > >      qxl_available_options,
> > > 
> > > Will xorg attempt to modify driverName? If not, I'd just add (char *) casts
> > > instead of creating a copy of QXL_DRIVER_NAME. Or is it customary to do
> > > things this way in X drivers?
> > 
> > No reason to assume it would change it, this is purely to remove a
> > warning - I know I'm doing it the wrong way, but (char *) doesn't fix
> > the warning. I could remove the -W flag used, but I don't like that
> > solution.
> 
> Ah, what is the warning exactly?
> 

My bad for not putting it in. It's warning on a const attribute removal
cast, since a static string "bla" is (const char*).

> Christophe
> 
> > It can't be stack allocated since it's used later, so I think
> > this is actually right (changing my mind from earlier this paragraph).
> > 
> > > 
> > > Christophe
> > 
> > 



> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On Tue, Apr 10, 2012 at 04:02:56PM +0300, Alon Levy wrote:
> My bad for not putting it in. It's warning on a const attribute removal
> cast, since a static string "bla" is (const char*).

For the record, the warning is -Wcast-qual. While this patch is okayish,
the 2 other similar patches are a bit ugly imo. Maybe we could use a pragma
to disable this warning around the initialisation of X structures?

Christophe
On Wed, Apr 11, 2012 at 12:46:18PM +0200, Christophe Fergeau wrote:
> On Tue, Apr 10, 2012 at 04:02:56PM +0300, Alon Levy wrote:
> > My bad for not putting it in. It's warning on a const attribute removal
> > cast, since a static string "bla" is (const char*).
> 
> For the record, the warning is -Wcast-qual. While this patch is okayish,
> the 2 other similar patches are a bit ugly imo. Maybe we could use a pragma
> to disable this warning around the initialisation of X structures?
> 
> Christophe

Which pragma does that?

> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On Wed, Apr 11, 2012 at 02:05:17PM +0300, Alon Levy wrote:
> On Wed, Apr 11, 2012 at 12:46:18PM +0200, Christophe Fergeau wrote:
> > On Tue, Apr 10, 2012 at 04:02:56PM +0300, Alon Levy wrote:
> > > My bad for not putting it in. It's warning on a const attribute removal
> > > cast, since a static string "bla" is (const char*).
> > 
> > For the record, the warning is -Wcast-qual. While this patch is okayish,
> > the 2 other similar patches are a bit ugly imo. Maybe we could use a pragma
> > to disable this warning around the initialisation of X structures?
> > 
> > Christophe
> 
> Which pragma does that?

Dunno, but actually I was wrong about the flag that causes these warnings,
it's -Wwrite-strings actually.
gcc documentation says:
-Wwrite-strings:
           When compiling C, give string constants the type "const
char[length]" so that copying the address of one into a non-"const" "char
*" pointer will get a warning.  These warnings will help you find at
compile time code that can try to write into a string constant, but only if
you have been very careful about using "const" in declarations and
prototypes.  Otherwise, it will just be a nuisance. This is why we did not
make -Wall request these warnings.

Given that we are definitely dealing with code not using "const"
consistently in declarations and prototypes, I'd tend to just drop that
warning.

Christophe
On Wed, Apr 11, 2012 at 01:37:49PM +0200, Christophe Fergeau wrote:
> On Wed, Apr 11, 2012 at 02:05:17PM +0300, Alon Levy wrote:
> > On Wed, Apr 11, 2012 at 12:46:18PM +0200, Christophe Fergeau wrote:
> > > On Tue, Apr 10, 2012 at 04:02:56PM +0300, Alon Levy wrote:
> > > > My bad for not putting it in. It's warning on a const attribute removal
> > > > cast, since a static string "bla" is (const char*).
> > > 
> > > For the record, the warning is -Wcast-qual. While this patch is okayish,
> > > the 2 other similar patches are a bit ugly imo. Maybe we could use a pragma
> > > to disable this warning around the initialisation of X structures?
> > > 
> > > Christophe
> > 
> > Which pragma does that?
> 
> Dunno, but actually I was wrong about the flag that causes these warnings,
> it's -Wwrite-strings actually.
> gcc documentation says:
> -Wwrite-strings:
>            When compiling C, give string constants the type "const
> char[length]" so that copying the address of one into a non-"const" "char
> *" pointer will get a warning.  These warnings will help you find at
> compile time code that can try to write into a string constant, but only if
> you have been very careful about using "const" in declarations and
> prototypes.  Otherwise, it will just be a nuisance. This is why we did not
> make -Wall request these warnings.
> 
> Given that we are definitely dealing with code not using "const"
> consistently in declarations and prototypes, I'd tend to just drop that
> warning.

My problem with carying on this seamingly simple suggestion is that I
haven't found the source of the warnings yet, I think it's via
xorg-macros, if I could disable this I'd also disable -Winline at the
same time.

> 
> Christophe



> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On Wed, Apr 11, 2012 at 03:04:37PM +0300, Alon Levy wrote:
> On Wed, Apr 11, 2012 at 01:37:49PM +0200, Christophe Fergeau wrote:
> > On Wed, Apr 11, 2012 at 02:05:17PM +0300, Alon Levy wrote:
> > > On Wed, Apr 11, 2012 at 12:46:18PM +0200, Christophe Fergeau wrote:
> > > > On Tue, Apr 10, 2012 at 04:02:56PM +0300, Alon Levy wrote:
> > > > > My bad for not putting it in. It's warning on a const attribute removal
> > > > > cast, since a static string "bla" is (const char*).
> > > > 
> > > > For the record, the warning is -Wcast-qual. While this patch is okayish,
> > > > the 2 other similar patches are a bit ugly imo. Maybe we could use a pragma
> > > > to disable this warning around the initialisation of X structures?
> > > > 
> > > > Christophe
> > > 
> > > Which pragma does that?
> > 
> > Dunno, but actually I was wrong about the flag that causes these warnings,
> > it's -Wwrite-strings actually.
> > gcc documentation says:
> > -Wwrite-strings:
> >            When compiling C, give string constants the type "const
> > char[length]" so that copying the address of one into a non-"const" "char
> > *" pointer will get a warning.  These warnings will help you find at
> > compile time code that can try to write into a string constant, but only if
> > you have been very careful about using "const" in declarations and
> > prototypes.  Otherwise, it will just be a nuisance. This is why we did not
> > make -Wall request these warnings.
> > 
> > Given that we are definitely dealing with code not using "const"
> > consistently in declarations and prototypes, I'd tend to just drop that
> > warning.
> 
> My problem with carying on this seamingly simple suggestion is that I
> haven't found the source of the warnings yet, I think it's via
> xorg-macros, if I could disable this I'd also disable -Winline at the
> same time.

They come from the XORG_COMPILER_FLAGS macro from
/usr/share/aclocal/xorg-macros.m4 which is called by XORG_DEFAULT_OPTIONS
in configure.ac. Since xorg is using this flag too, it would be interesting
to see if they got similar errors, and how they fixed them. We can just
append -Wno-inline -Wno-write-strings to CWARNFLAGS to disable these
warnings in qxl I think.
However, I won't ACK such a change, I'd prefer to know how ssp feels about
it first.

Christophe