RFA: coding style tweaks

RFA: coding style tweaks

[Jim Blandy]

2002-05-13  Jim Blandy  <jimb@redhat.com>

	* macroexp.c (init_buffer, gather_arguments, expand): Use NULL, not 0.
	* macrotab.c (macro_lookup_inclusion, find_definition,
	new_macro_table): Same.
	
	* macroexp.c (currently_rescanning, expand): Use `strcmp () == 0',
	not `! strcmp ()'.  This is a dubious improvement.
	* macrotab.c (macro_lookup_inclusion, find_definition): Same.

	* macrotab (macro_lookup_inclusion): Initialize `best_depth',
	although it's not necessary, to avoid a warning.

Index: gdb/macroexp.c
===================================================================
RCS file: /cvs/src/src/gdb/macroexp.c,v
retrieving revision 1.2
diff -c -r1.2 macroexp.c
*** gdb/macroexp.c	13 May 2002 18:13:07 -0000	1.2
--- gdb/macroexp.c	13 May 2002 23:49:25 -0000
***************
*** 88,94 ****
    if (n > 0)
      b->text = (char *) xmalloc (n);
    else
!     b->text = 0;
    b->len = 0;
    b->shared = 0;
    b->last_token = -1;
--- 88,94 ----
    if (n > 0)
      b->text = (char *) xmalloc (n);
    else
!     b->text = NULL;
    b->len = 0;
    b->shared = 0;
    b->last_token = -1;
***************
*** 646,652 ****
  currently_rescanning (struct macro_name_list *list, const char *name)
  {
    for (; list; list = list->next)
!     if (! strcmp (name, list->name))
        return 1;
  
    return 0;
--- 646,652 ----
  currently_rescanning (struct macro_name_list *list, const char *name)
  {
    for (; list; list = list->next)
!     if (strcmp (name, list->name) == 0)
        return 1;
  
    return 0;
***************
*** 692,698 ****
  {
    struct macro_buffer tok;
    int args_len, args_size;
!   struct macro_buffer *args = 0;
    struct cleanup *back_to = make_cleanup (free_current_contents, &args);
  
    /* Does SRC start with an opening paren token?  Read from a copy of
--- 692,698 ----
  {
    struct macro_buffer tok;
    int args_len, args_size;
!   struct macro_buffer *args = NULL;
    struct cleanup *back_to = make_cleanup (free_current_contents, &args);
  
    /* Does SRC start with an opening paren token?  Read from a copy of
***************
*** 928,939 ****
      {
        struct cleanup *back_to = make_cleanup (null_cleanup, 0);
        int argc;
!       struct macro_buffer *argv = 0;
        struct macro_buffer substituted;
        struct macro_buffer substituted_src;
  
        if (def->argc >= 1
!           && ! strcmp (def->argv[def->argc - 1], "..."))
          error ("Varargs macros not implemented yet.");
  
        make_cleanup (free_current_contents, &argv);
--- 928,939 ----
      {
        struct cleanup *back_to = make_cleanup (null_cleanup, 0);
        int argc;
!       struct macro_buffer *argv = NULL;
        struct macro_buffer substituted;
        struct macro_buffer substituted_src;
  
        if (def->argc >= 1
!           && strcmp (def->argv[def->argc - 1], "...") == 0)
          error ("Varargs macros not implemented yet.");
  
        make_cleanup (free_current_contents, &argv);
Index: gdb/macrotab.c
===================================================================
RCS file: /cvs/src/src/gdb/macrotab.c,v
retrieving revision 1.2
diff -c -r1.2 macrotab.c
*** gdb/macrotab.c	13 May 2002 18:13:07 -0000	1.2
--- gdb/macrotab.c	13 May 2002 23:49:26 -0000
***************
*** 479,485 ****
  macro_lookup_inclusion (struct macro_source_file *source, const char *name)
  {
    /* Is SOURCE itself named NAME?  */
!   if (! strcmp (name, source->filename))
      return source;
  
    /* The filename in the source structure is probably a full path, but
--- 479,485 ----
  macro_lookup_inclusion (struct macro_source_file *source, const char *name)
  {
    /* Is SOURCE itself named NAME?  */
!   if (strcmp (name, source->filename) == 0)
      return source;
  
    /* The filename in the source structure is probably a full path, but
***************
*** 493,507 ****
         check for a slash here.  */
      if (name_len < src_name_len
          && source->filename[src_name_len - name_len - 1] == '/'
!         && ! strcmp (name, source->filename + src_name_len - name_len))
        return source;
    }
  
    /* It's not us.  Try all our children, and return the lowest.  */
    {
      struct macro_source_file *child;
!     struct macro_source_file *best = 0;
!     int best_depth;
  
      for (child = source->includes; child; child = child->next_included)
        {
--- 493,507 ----
         check for a slash here.  */
      if (name_len < src_name_len
          && source->filename[src_name_len - name_len - 1] == '/'
!         && strcmp (name, source->filename + src_name_len - name_len) == 0)
        return source;
    }
  
    /* It's not us.  Try all our children, and return the lowest.  */
    {
      struct macro_source_file *child;
!     struct macro_source_file *best = NULL;
!     int best_depth = 0;
  
      for (child = source->includes; child; child = child->next_included)
        {
***************
*** 618,624 ****
    query.name = name;
    query.start_file = file;
    query.start_line = line;
!   query.end_file = 0;
  
    n = splay_tree_lookup (t->definitions, (splay_tree_key) &query);
    if (! n)
--- 618,624 ----
    query.name = name;
    query.start_file = file;
    query.start_line = line;
!   query.end_file = NULL;
  
    n = splay_tree_lookup (t->definitions, (splay_tree_key) &query);
    if (! n)
***************
*** 638,644 ****
               We just want to search within a given name's definitions.  */
            struct macro_key *found = (struct macro_key *) pred->key;
  
!           if (! strcmp (found->name, name))
              n = pred;
          }
      }
--- 638,644 ----
               We just want to search within a given name's definitions.  */
            struct macro_key *found = (struct macro_key *) pred->key;
  
!           if (strcmp (found->name, name) == 0)
              n = pred;
          }
      }
***************
*** 838,844 ****
    memset (t, 0, sizeof (*t));
    t->obstack = obstack;
    t->bcache = b;
!   t->main_source = 0;
    t->definitions = (splay_tree_new_with_allocator
                      (macro_tree_compare,
                       ((splay_tree_delete_key_fn) macro_tree_delete_key),
--- 838,844 ----
    memset (t, 0, sizeof (*t));
    t->obstack = obstack;
    t->bcache = b;
!   t->main_source = NULL;
    t->definitions = (splay_tree_new_with_allocator
                      (macro_tree_compare,
                       ((splay_tree_delete_key_fn) macro_tree_delete_key),

Re: RFA: coding style tweaks

[Daniel Berlin]

>     }
>   
>     /* It's not us.  Try all our children, and return the lowest.  */
>     {
>       struct macro_source_file *child;
> !     struct macro_source_file *best = NULL;
> !     int best_depth = 0;
>   
>       for (child = source->includes; child; child = child->next_included)
>         {

This has to be wrong.

If you initialize best_depth to be 0, unless result_depth is somehow < 0, 
it'll never be the new best.
--Dan

Re: RFA: coding style tweaks

[Andrew Cagney]

> for the code:
>> 
>>     /* It's not us.  Try all our children, and return the lowest.  */
>>     {
>>       struct macro_source_file *child;
>>       struct macro_source_file *best = 0;
>>       int best_depth;
>> 
>>       for (child = source->includes; child; child = child->next_included)
>>         {
>>           struct macro_source_file *result
>>             = macro_lookup_inclusion (child, name);
>> 
>>           if (result)
>>             {
>>               int result_depth = inclusion_depth (result);
>> 
>>               if (! best || result_depth < best_depth) <-- HERE
> 
> 
> It's an obvious false positive (!best will be true the first time through, 
> meaning the only time we check best_depth, it's already been set at 
> least once).

(I know it is a ``false positive'' but then again if GCC can't figure it 
out, how will I :-)

> Here, you can't just initialize best_depth to 0, you have to initialize it 
> to either INT_MAX, or inclusion_depth (result).

Since || is a short-circuit, the RHS really doesn't matter.
INT_MAX, though, would make it clearer, could even drop ``!best''.

enjoy,
Andrew

Re: RFA: coding style tweaks

[Daniel Berlin]

On Mon, 13 May 2002, Andrew Cagney wrote:

> > for the code:
> >> 
> >>     /* It's not us.  Try all our children, and return the lowest.  */
> >>     {
> >>       struct macro_source_file *child;
> >>       struct macro_source_file *best = 0;
> >>       int best_depth;
> >> 
> >>       for (child = source->includes; child; child = child->next_included)
> >>         {
> >>           struct macro_source_file *result
> >>             = macro_lookup_inclusion (child, name);
> >> 
> >>           if (result)
> >>             {
> >>               int result_depth = inclusion_depth (result);
> >> 
> >>               if (! best || result_depth < best_depth) <-- HERE
> > 
> > 
> > It's an obvious false positive (!best will be true the first time through, 
> > meaning the only time we check best_depth, it's already been set at 
> > least once).
> 
> (I know it is a ``false positive'' but then again if GCC can't figure it 
> out, how will I :-)

Depends on which version of gcc. Some can figure it out.
:)
Fun, isn't -Wuninitialized?


> 
> > Here, you can't just initialize best_depth to 0, you have to initialize it 
> > to either INT_MAX, or inclusion_depth (result).
> 
> Since || is a short-circuit, the RHS really doesn't matter.
Yeah, yer right.

> INT_MAX, though, would make it clearer, could even drop ``!best''.

And 3 years from now, when someone decides to rewrite this code, they 
won't break it if they decide to transform that way.


> 
> enjoy,
> Andrew
> 
> 
> 
> 
> 

Re: RFA: coding style tweaks

[Andrew Cagney]

> Here, you can't just initialize best_depth to 0, you have to initialize it 
>> > to either INT_MAX, or inclusion_depth (result).
> 
>> 
>> Since || is a short-circuit, the RHS really doesn't matter.
> 
> Yeah, yer right.

I should point out that I had to look this up in my tartan labs book! :-)

Andrew

Re: RFA: coding style tweaks

[Jim Blandy]

I've committed these, as obvious fixes.

Jim Blandy <jimb@redhat.com> writes:

> 2002-05-13  Jim Blandy  <jimb@redhat.com>
> 
> 	* macroexp.c (init_buffer, gather_arguments, expand): Use NULL, not 0.
> 	* macrotab.c (macro_lookup_inclusion, find_definition,
> 	new_macro_table): Same.
> 	
> 	* macroexp.c (currently_rescanning, expand): Use `strcmp () == 0',
> 	not `! strcmp ()'.  This is a dubious improvement.
> 	* macrotab.c (macro_lookup_inclusion, find_definition): Same.
> 
> 	* macrotab (macro_lookup_inclusion): Initialize `best_depth',
> 	although it's not necessary, to avoid a warning.