[RFA] p-typeprint.c, move pointer use to after null-check.

[RFA] p-typeprint.c, move pointer use to after null-check.

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 41 bytes --]

If it's worth checking for null...

OK?


[-- Attachment #2: reversenull2.txt --]
[-- Type: text/plain, Size: 1180 bytes --]

2011-03-01  Michael Snyder  <msnyder@vmware.com>

	* p-typeprint.c (pascal_type_print_method_args): Don't use 
	pointer until after null-check.

Index: p-typeprint.c
===================================================================
RCS file: /cvs/src/src/gdb/p-typeprint.c,v
retrieving revision 1.38
diff -u -p -u -p -r1.38 p-typeprint.c
--- p-typeprint.c	10 Jan 2011 20:38:50 -0000	1.38
+++ p-typeprint.c	1 Mar 2011 20:34:08 -0000
@@ -156,18 +156,18 @@ void
 pascal_type_print_method_args (char *physname, char *methodname,
 			       struct ui_file *stream)
 {
-  int is_constructor = (strncmp (physname, "__ct__", 6) == 0);
-  int is_destructor = (strncmp (physname, "__dt__", 6) == 0);
-
-  if (is_constructor || is_destructor)
-    {
-      physname += 6;
-    }
-
   fputs_filtered (methodname, stream);
 
   if (physname && (*physname != 0))
     {
+      int is_constructor = (strncmp (physname, "__ct__", 6) == 0);
+      int is_destructor = (strncmp (physname, "__dt__", 6) == 0);
+
+      if (is_constructor || is_destructor)
+	{
+	  physname += 6;
+	}
+
       fputs_filtered (" (", stream);
       /* We must demangle this.  */
       while (isdigit (physname[0]))

Re: [RFA] p-typeprint.c, move pointer use to after null-check.

[Michael Snyder]

Michael Snyder wrote:
> If it's worth checking for null...
> 
> OK?
> 

Ping?

Re: [RFA] p-typeprint.c, move pointer use to after null-check.

[Joel Brobecker]

> If it's worth checking for null...
> 
> OK?
> 

> 2011-03-01  Michael Snyder  <msnyder@vmware.com>
> 
> 	* p-typeprint.c (pascal_type_print_method_args): Don't use 
> 	pointer until after null-check.

I think I get the drift of the code, and ISTM that the check for NULL
might be misleading. I think that "physname" can never be null, by
virtue of how it's called. What I would do is just remove the check
against NULL (we can add a gdb_assert at the same time, which would
force us to declare the is_constructor/is_destructor variables without
initial value - no big deal).

Pierre?

A few remarks:

The function could (should?) be made static, unless I grep'ed wrong

> +      if (is_constructor || is_destructor)
> +	{
> +	  physname += 6;
> +	}

Useless extra curly braces...

-- 
Joel

RE: [RFA] p-typeprint.c, move pointer use to after null-check.

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : mardi 8 mars 2011 06:10
> À : Michael Snyder
> Cc : gdb-patches@sourceware.org; Pierre Muller
> Objet : Re: [RFA] p-typeprint.c, move pointer use to after null-check.
> 
> > If it's worth checking for null...
> >
> > OK?
> >
> 
> > 2011-03-01  Michael Snyder  <msnyder@vmware.com>
> >
> > 	* p-typeprint.c (pascal_type_print_method_args): Don't use
> > 	pointer until after null-check.
> I think I get the drift of the code, and ISTM that the check for NULL
> might be misleading. I think that "physname" can never be null, by
> virtue of how it's called. What I would do is just remove the check
> against NULL (we can add a gdb_assert at the same time, which would
> force us to declare the is_constructor/is_destructor variables without
> initial value - no big deal).
> 
> Pierre?

  No, I think that the code relies on the fact that physname
is never null, but that the constructor or destructor could have no
parameters, in which case  physname would point to \0
after the '+= 6'.
  Parameterless pascal procedure/functions
do not require opening/closing braces like C does.
  So I still think that the current code is correct.
 So to answer to Michael, with my apologies for the delay,
I do think that your patch is not correct and should
not be applied as is.
 
> A few remarks:
> 
> The function could (should?) be made static, unless I grep'ed wrong
  Should indeed...
  
 
> > +      if (is_constructor || is_destructor)
> > +	{
> > +	  physname += 6;
> > +	}
> 
> Useless extra curly braces...
 OK, while correcting these, I also saw that:  

Is this correct:
   if (physname && (*physname != 0))
or is:
   if (physname && *physname != 0)

better?
or should I use:
   if (physname && *physname != '\0')


Pierre Muller
GDB pascal language maintainer

Re: [RFA] p-typeprint.c, move pointer use to after null-check.

[Joel Brobecker]

>   No, I think that the code relies on the fact that physname
> is never null, but that the constructor or destructor could have no
> parameters, in which case  physname would point to \0
> after the '+= 6'.

OK, we agree, in fact:

> Is this correct:
>    if (physname && (*physname != 0))
> or is:
>    if (physname && *physname != 0)
> 
> better?
> or should I use:
>    if (physname && *physname != '\0')

My suggestion was to remove the "physname [!= NULL]" part only.
Hence:

    if (*physname != '\0')

(I think we have more cases of '\0' than 0 when we're checking
character values)

-- 
Joel