Re: [PATCH:libXt 4/6] Fix char vs. unsigned char warnings.

[Thomas Klausner]

On Wed, Jun 26, 2013 at 08:55:35PM -0400, Thomas E. Dickey wrote:
 On Tue, Jun 25, 2013 at 11:02:48PM +0200, Thomas Klausner wrote:
  ---
   src/ResConfig.c | 4 ++--
   src/TMparse.c   | 4 ++--
   2 files changed, 4 insertions(+), 4 deletions(-)
  
  diff --git a/src/ResConfig.c b/src/ResConfig.c
  index 152d9cf..5a7f6d2 100644
  --- a/src/ResConfig.c
  +++ b/src/ResConfig.c
  @@ -892,7 +892,7 @@ _XtResourceConfigurationEH (
  int actual_format;
  unsigned long   nitems;
  unsigned long   leftover;
  -   unsigned char   *data = NULL;
  +   char*data = NULL;
  unsigned long   resource_len;
  char*data_ptr;
  char*resource;
  @@ -952,7 +952,7 @@ _XtResourceConfigurationEH (
  pd-rcm_data, 0L, 8192L,
  TRUE, XA_STRING,
  actual_type, actual_format, nitems, leftover,
  -   data ) == Success  actual_type == XA_STRING
  +   (unsigned char **)data ) == Success  actual_type == XA_STRING
  actual_format == 8) {
 
 One problem with casts is that they're telling the compiler to ignore its 
 type-checking.
 Casting an address like that happens to work - usually - but not always.

The problem is that the same variable is used in strtoul as first
argument, which wants a 'const char *', and in XGetWindowProperty as
12th (if I didn't miscount), which wants an unsigned char **.

i.e.

if (XGetWindowProperty (XtDisplay(w),
XtWindow (w),
pd-rcm_data, 0L, 8192L,
TRUE, XA_STRING,
actual_type, actual_format, nitems, leftover,
(unsigned char **)data ) == Success  actual_type == XA_STRING
actual_format == 8) {
...
resource_len = strtoul (data, data_ptr, 10);

How would you suggest fixing issues like that?
 Thomas

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH:libXt 4/6] Fix char vs. unsigned char warnings.

[Thomas Dickey]

On Thu, Jun 27, 2013 at 08:33:02AM +0200, Thomas Klausner wrote:
 On Wed, Jun 26, 2013 at 08:55:35PM -0400, Thomas E. Dickey wrote:
  On Tue, Jun 25, 2013 at 11:02:48PM +0200, Thomas Klausner wrote:
   ---
src/ResConfig.c | 4 ++--
src/TMparse.c   | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
   
   diff --git a/src/ResConfig.c b/src/ResConfig.c
   index 152d9cf..5a7f6d2 100644
   --- a/src/ResConfig.c
   +++ b/src/ResConfig.c
   @@ -892,7 +892,7 @@ _XtResourceConfigurationEH (
 int actual_format;
 unsigned long   nitems;
 unsigned long   leftover;
   - unsigned char   *data = NULL;
   + char*data = NULL;
 unsigned long   resource_len;
 char*data_ptr;
 char*resource;
   @@ -952,7 +952,7 @@ _XtResourceConfigurationEH (
 pd-rcm_data, 0L, 8192L,
 TRUE, XA_STRING,
 actual_type, actual_format, nitems, leftover,
   - data ) == Success  actual_type == XA_STRING
   + (unsigned char **)data ) == Success  actual_type == XA_STRING
 actual_format == 8) {
  
  One problem with casts is that they're telling the compiler to ignore its 
  type-checking.
  Casting an address like that happens to work - usually - but not always.
 
 The problem is that the same variable is used in strtoul as first
 argument, which wants a 'const char *', and in XGetWindowProperty as
 12th (if I didn't miscount), which wants an unsigned char **.

But taking the address of a pointer tends to get additional compiler warnings 
cautioning
about alignment, etc.  I would have probably put the cast on the strtoul call.  
You might
want to compare results from these -

OPTS=-O -Wall -Wmissing-prototypes -Wstrict-prototypes -Wshadow -Wconversion
-W
-Wbad-function-cast
-Wcast-align
-Wcast-qual
-Wmissing-declarations
-Wnested-externs
-Wpointer-arith
-Wwrite-strings
-ansi
-pedantic
 
gcc $OPTS $@

versus

OPTS=-Wall -Wstrict-prototypes -Wmissing-prototypes -Wshadow -Wconversion
gcc $OPTS $@

-- 
Thomas E. Dickey dic...@invisible-island.net
http://invisible-island.net
ftp://invisible-island.net


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH:libXt 4/6] Fix char vs. unsigned char warnings.

[walter harms]

Am 27.06.2013 08:33, schrieb Thomas Klausner:
 On Wed, Jun 26, 2013 at 08:55:35PM -0400, Thomas E. Dickey wrote:
 On Tue, Jun 25, 2013 at 11:02:48PM +0200, Thomas Klausner wrote:
 ---
  src/ResConfig.c | 4 ++--
  src/TMparse.c   | 4 ++--
  2 files changed, 4 insertions(+), 4 deletions(-)

 diff --git a/src/ResConfig.c b/src/ResConfig.c
 index 152d9cf..5a7f6d2 100644
 --- a/src/ResConfig.c
 +++ b/src/ResConfig.c
 @@ -892,7 +892,7 @@ _XtResourceConfigurationEH (
 int actual_format;
 unsigned long   nitems;
 unsigned long   leftover;
 -   unsigned char   *data = NULL;
 +   char*data = NULL;
 unsigned long   resource_len;
 char*data_ptr;
 char*resource;
 @@ -952,7 +952,7 @@ _XtResourceConfigurationEH (
 pd-rcm_data, 0L, 8192L,
 TRUE, XA_STRING,
 actual_type, actual_format, nitems, leftover,
 -   data ) == Success  actual_type == XA_STRING
 +   (unsigned char **)data ) == Success  actual_type == XA_STRING
 actual_format == 8) {

 One problem with casts is that they're telling the compiler to ignore its 
 type-checking.
 Casting an address like that happens to work - usually - but not always.
 
 The problem is that the same variable is used in strtoul as first
 argument, which wants a 'const char *', and in XGetWindowProperty as
 12th (if I didn't miscount), which wants an unsigned char **.
 
 i.e.
 
 if (XGetWindowProperty (XtDisplay(w),
 XtWindow (w),
 pd-rcm_data, 0L, 8192L,
 TRUE, XA_STRING,
 actual_type, actual_format, nitems, leftover,
 (unsigned char **)data ) == Success  actual_type == 
 XA_STRING
 actual_format == 8) {
 ...
 resource_len = strtoul (data, data_ptr, 10);
 
 How would you suggest fixing issues like that?
  Thomas
 

i would suggest to simplify the code first by making two lines.
That would improve readablity exspecially since XGetWindowProperty()
takes a lot of arguments.

ret=XGetWindowProperty(..);
if ( ret == Success  actual_type == XA_STRING   actual_format == 8) {

And when you need a cast anyway why not using a void * ?

just my 2 cents,
 wh


___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH:libXt 4/6] Fix char vs. unsigned char warnings.

[Thomas Klausner]

On Thu, Jun 27, 2013 at 04:23:06AM -0400, Thomas E. Dickey wrote:
 On Thu, Jun 27, 2013 at 08:33:02AM +0200, Thomas Klausner wrote:
  On Wed, Jun 26, 2013 at 08:55:35PM -0400, Thomas E. Dickey wrote:
   On Tue, Jun 25, 2013 at 11:02:48PM +0200, Thomas Klausner wrote:
---
 src/ResConfig.c | 4 ++--
 src/TMparse.c   | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/ResConfig.c b/src/ResConfig.c
index 152d9cf..5a7f6d2 100644
--- a/src/ResConfig.c
+++ b/src/ResConfig.c
@@ -892,7 +892,7 @@ _XtResourceConfigurationEH (
int actual_format;
unsigned long   nitems;
unsigned long   leftover;
-   unsigned char   *data = NULL;
+   char*data = NULL;
unsigned long   resource_len;
char*data_ptr;
char*resource;
@@ -952,7 +952,7 @@ _XtResourceConfigurationEH (
pd-rcm_data, 0L, 8192L,
TRUE, XA_STRING,
actual_type, actual_format, nitems, leftover,
-   data ) == Success  actual_type == XA_STRING
+   (unsigned char **)data ) == Success  actual_type == 
XA_STRING
actual_format == 8) {
   
   One problem with casts is that they're telling the compiler to ignore its 
   type-checking.
   Casting an address like that happens to work - usually - but not always.
  
  The problem is that the same variable is used in strtoul as first
  argument, which wants a 'const char *', and in XGetWindowProperty as
  12th (if I didn't miscount), which wants an unsigned char **.
 
 But taking the address of a pointer tends to get additional compiler warnings 
 cautioning
 about alignment, etc.  I would have probably put the cast on the strtoul call.

So that we all know what we're talking about, I wanted to fix this
complaint from clang-3.4 (against the code without my patch):

ResConfig.c:967:10: warning: initializing 'char *' with an expression of type 
'unsigned char *' converts between pointers to integer types with different 
sign [-Wpointer-sign]
char *data_end = data + nitems;
  ^  ~
ResConfig.c:970:28: warning: passing 'unsigned char *' to parameter of type 
'const char *' converts between pointers to integer types with different sign 
[-Wpointer-sign]
resource_len = strtoul (data, data_ptr, 10);
^~~~
/usr/include/stdlib.h:125:34: note: passing argument to parameter here
 strtoul(const char * __restrict, char ** __restrict, int);
^
2 warnings generated.


The diff I sent is the smallest that gets rid of this issue. I also
tried leaving the type of 'data' alone, but then other variables need
to be changed and the diff gets larger, and we still need casts for
the reason above.

As for your question:

 You might
 want to compare results from these -
 
 OPTS=-O -Wall -Wmissing-prototypes -Wstrict-prototypes -Wshadow -Wconversion
 -W
   -Wbad-function-cast
   -Wcast-align
   -Wcast-qual
   -Wmissing-declarations
   -Wnested-externs
   -Wpointer-arith
   -Wwrite-strings
   -ansi
   -pedantic
  
 gcc $OPTS $@

  CC   ResConfig.lo
In file included from /usr/pkg/include/X11/Xlib.h:47:0,
 from ../include/X11/Intrinsic.h:53,
 from ResConfig.c:58:
/usr/pkg/include/X11/Xfuncproto.h:136:24: warning: ISO C does not permit named 
variadic macros
ResConfig.c: In function '_set_resource_values':
ResConfig.c:264:3: warning: cast discards qualifiers from pointer target type
ResConfig.c: In function '_search_widget_tree':
ResConfig.c:705:2: warning: conversion to 'int' from 'size_t' may alter its 
value
ResConfig.c:706:2: warning: conversion to 'int' from 'size_t' may alter its 
value
ResConfig.c: In function '_locate_children':
ResConfig.c:777:3: warning: conversion to 'Cardinal' from 'int' may change the 
sign of the result
ResConfig.c:777:3: warning: conversion to 'int' from 'Cardinal' may change the 
sign of the result
ResConfig.c:779:3: warning: conversion to 'Cardinal' from 'int' may change the 
sign of the result
ResConfig.c:779:3: warning: conversion to 'int' from 'Cardinal' may change the 
sign of the result
ResConfig.c:786:3: warning: conversion to 'unsigned int' from 'int' may change 
the sign of the result

 versus
 
 OPTS=-Wall -Wstrict-prototypes -Wmissing-prototypes -Wshadow -Wconversion
 gcc $OPTS $@

  CC   ResConfig.lo
ResConfig.c: In function '_set_resource_values':
ResConfig.c:264:3: warning: cast discards qualifiers from pointer target type
ResConfig.c: In function '_search_widget_tree':
ResConfig.c:705:2: warning: conversion to 'int' from 'size_t' may alter its 
value
ResConfig.c:706:2: warning: conversion to 'int' from 'size_t' may alter 

Re: [PATCH:libXt 4/6] Fix char vs. unsigned char warnings.

[Thomas Dickey]

On Tue, Jun 25, 2013 at 11:02:48PM +0200, Thomas Klausner wrote:
 ---
  src/ResConfig.c | 4 ++--
  src/TMparse.c   | 4 ++--
  2 files changed, 4 insertions(+), 4 deletions(-)
 
 diff --git a/src/ResConfig.c b/src/ResConfig.c
 index 152d9cf..5a7f6d2 100644
 --- a/src/ResConfig.c
 +++ b/src/ResConfig.c
 @@ -892,7 +892,7 @@ _XtResourceConfigurationEH (
   int actual_format;
   unsigned long   nitems;
   unsigned long   leftover;
 - unsigned char   *data = NULL;
 + char*data = NULL;
   unsigned long   resource_len;
   char*data_ptr;
   char*resource;
 @@ -952,7 +952,7 @@ _XtResourceConfigurationEH (
   pd-rcm_data, 0L, 8192L,
   TRUE, XA_STRING,
   actual_type, actual_format, nitems, leftover,
 - data ) == Success  actual_type == XA_STRING
 + (unsigned char **)data ) == Success  actual_type == XA_STRING
   actual_format == 8) {

One problem with casts is that they're telling the compiler to ignore its 
type-checking.
Casting an address like that happens to work - usually - but not always.

-- 
Thomas E. Dickey dic...@invisible-island.net
http://invisible-island.net
ftp://invisible-island.net


signature.asc
Description: Digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH:libXt 4/6] Fix char vs. unsigned char warnings.

[Thomas Klausner]

---
 src/ResConfig.c | 4 ++--
 src/TMparse.c   | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/ResConfig.c b/src/ResConfig.c
index 152d9cf..5a7f6d2 100644
--- a/src/ResConfig.c
+++ b/src/ResConfig.c
@@ -892,7 +892,7 @@ _XtResourceConfigurationEH (
int actual_format;
unsigned long   nitems;
unsigned long   leftover;
-   unsigned char   *data = NULL;
+   char*data = NULL;
unsigned long   resource_len;
char*data_ptr;
char*resource;
@@ -952,7 +952,7 @@ _XtResourceConfigurationEH (
pd-rcm_data, 0L, 8192L,
TRUE, XA_STRING,
actual_type, actual_format, nitems, leftover,
-   data ) == Success  actual_type == XA_STRING
+   (unsigned char **)data ) == Success  actual_type == XA_STRING
actual_format == 8) {
/*
 *  data format is:
diff --git a/src/TMparse.c b/src/TMparse.c
index 83b39d5..df94181 100644
--- a/src/TMparse.c
+++ b/src/TMparse.c
@@ -1472,10 +1472,10 @@ static String ParseRepeat(
 {
 
 /*** Parse the repetitions, for double click etc... ***/
-if (*str != '(' || !(isdigit(str[1]) || str[1] == '+' || str[1] == ')'))
+if (*str != '(' || !(isdigit((unsigned char)str[1]) || str[1] == '+' || 
str[1] == ')'))
return str;
 str++;
-if (isdigit(*str)) {
+if (isdigit((unsigned char)*str)) {
String start = str;
char repStr[7];
size_t len;
-- 
1.8.3.1

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel