[patch/ob]

[patch/ob]

[Andrew Cagney]

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

Just FYI,

I've checked the attached in as, er, obvious.  It fixes a
-Wuninitialized warning.

	Andrew


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

2001-12-11  Andrew Cagney  <ac131313@redhat.com>

	* values.c: Include "gdb_assert.h".
	(value_fn_field): Rearange to avoid -Wuninitialized warning.

Index: values.c
===================================================================
RCS file: /cvs/src/src/gdb/values.c,v
retrieving revision 1.30
diff -p -r1.30 values.c
*** values.c	2001/12/10 23:05:00	1.30
--- values.c	2001/12/11 18:05:09
***************
*** 33,38 ****
--- 33,39 ----
  #include "scm-lang.h"
  #include "demangle.h"
  #include "doublest.h"
+ #include "gdb_assert.h"
  
  /* Prototypes for exported functions. */
  
*************** value_fn_field (value_ptr *arg1p, struct
*** 971,983 ****
    struct minimal_symbol *msym;
  
    sym = lookup_symbol (physname, 0, VAR_NAMESPACE, 0, NULL);
!   if (!sym)
      {
        msym = lookup_minimal_symbol (physname, NULL, NULL);
      }
- 
-   if (!sym && !msym)
-     return NULL;
  
    v = allocate_value (ftype);
    if (sym)
--- 972,988 ----
    struct minimal_symbol *msym;
  
    sym = lookup_symbol (physname, 0, VAR_NAMESPACE, 0, NULL);
!   if (sym != NULL)
      {
+       msym = NULL;
+     }
+   else
+     {
+       gdb_assert (sym == NULL);
        msym = lookup_minimal_symbol (physname, NULL, NULL);
+       if (msym == NULL)
+ 	return NULL;
      }
  
    v = allocate_value (ftype);
    if (sym)

Re: [patch/ob]

[Daniel Jacobowitz]

On Tue, Dec 11, 2001 at 10:18:05AM -0800, Andrew Cagney wrote:
> Just FYI,
> 
> I've checked the attached in as, er, obvious.  It fixes a
> -Wuninitialized warning.

I agree with the "er, obvious".

Would there be anything wrong with:

> *************** value_fn_field (value_ptr *arg1p, struct
> *** 971,983 ****
>     struct minimal_symbol *msym;

adding = NULL to the line above?

>   
>     sym = lookup_symbol (physname, 0, VAR_NAMESPACE, 0, NULL);
> !   if (!sym)
>       {
>         msym = lookup_minimal_symbol (physname, NULL, NULL);
>       }
> - 
> -   if (!sym && !msym)
> -     return NULL;
>   
>     v = allocate_value (ftype);
>     if (sym)
> --- 972,988 ----
>     struct minimal_symbol *msym;
>   
>     sym = lookup_symbol (physname, 0, VAR_NAMESPACE, 0, NULL);
> !   if (sym != NULL)
>       {
> +       msym = NULL;
> +     }
> +   else
> +     {
> +       gdb_assert (sym == NULL);

This assert in particular bugs me.  Adding asserts that the compiler
can obviously eliminate, since sym isn't volatile...


-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/ob]

[Andrew Cagney]

> On Tue, Dec 11, 2001 at 10:18:05AM -0800, Andrew Cagney wrote:
> 
>> Just FYI,
>> 
>> I've checked the attached in as, er, obvious.  It fixes a
>> -Wuninitialized warning.
> 
> 
> I agree with the "er, obvious".
> 
> Would there be anything wrong with:
> 
> 
>> *************** value_fn_field (value_ptr *arg1p, struct
>> *** 971,983 ****
>> struct minimal_symbol *msym;
> 
> 
> adding = NULL to the line above?


6 of one, half a dozen of the other.

My personal preference is to do an initialization as:

	if (expr)
	  foo = val1;
	  bar = valb;
	  baz = valz;
	else if (expr2)
	  foo = val2;
	  bar = valxx;
	  bas = val3;
	else
	  foo = defaultval;
	  bar = devbar;
	  baz = baxdev;

rather than:

	bar = devbar;
	baz = baxdev;
	...
	...
	...
	...
	...
	...
	if (expr)
	  foo = val1;
	  bar = valb;
	else if (expr2)
	  foo = val2;
	  baz = val3;

This is because I consider the former to have clear pre/post assertions 
and exploits the compiler's -Wuninitialized facility - you know all 
branches have initialized foo, bar and baz (I'm not sure about the 
latter example mind :-).  This becomes especially useful when handling 
initialization in in very long switches and if/elif chains.

> +       gdb_assert (sym == NULL);
> 
> 
> This assert in particular bugs me.  Adding asserts that the compiler
> can obviously eliminate, since sym isn't volatile...


Here, sorry, I'm lost.  Assertions are added to code for many reasons - 
one being that it helps ``prove'' correctness, another is that it can 
clarify the intention and assumptions of the developer.  I don't think 
compiler has much to do with this.

Keep in mind that we use GCC's -Werror messages as a tool.  Not as an 
end in themselves.

enjoy,
Andrew