Re: gengtype improvements for plugins, thirdround! patch 5/7 [typedopt]

[Basile Starynkevitch <basile@starynkevitch.net>]

On Wed, Sep 22, 2010 at 05:46:29AM +0300, Laurynas Biveinis wrote:


A big and warm thanks to Laurynas for your review and for taking time
to read our patch. 

However, I believe that gengtype is misindented from the start, and
this explains most of my indentation issues. Please read
http://gcc.gnu.org/ml/gcc/2010-09/msg00448.html for details.


> 2010/9/21 Basile Starynkevitch <basile@starynkevitch.net>:
> 
> @@ -1082,14 +1018,11 @@ adjust_field_rtx_def (type_p t, options_
>    /* Create a type to represent the various forms of SYMBOL_REF_DATA.  */
>    {
>      pair_p sym_flds;
> -
> -    sym_flds = create_field (NULL, tree_tp, "rt_tree");
> -    sym_flds->opt = create_option (nodot, "default", "");
> -
> -    sym_flds = create_field (sym_flds, constant_tp, "rt_constant");
> -    sym_flds->opt = create_option (nodot, "tag", "1");
> -
> -    symbol_union_tp = new_structure ("rtx_def_symbol_subunion", 1,
> +     sym_flds = create_field (NULL, tree_tp, "rt_tree");
> +    sym_flds->opt = create_string_option (nodot, "default", "");
> +     sym_flds = create_field (sym_flds, constant_tp, "rt_constant");
> +    sym_flds->opt = create_string_option (nodot, "tag", "1");
> +     symbol_union_tp = new_structure ("rtx_def_symbol_subunion", 1,
>  				     &lexer_line, sym_flds, NULL);
>    }
>    for (i = 0; i < NUM_RTX_CODE; i++)
> 
> Indentation is wrong.
> 
> @@ -1228,12 +1161,10 @@ adjust_field_rtx_def (type_p t, options_
>        ftag = xstrdup (rtx_name[i]);
>        for (nmindex = 0; nmindex < strlen (ftag); nmindex++)
>  	ftag[nmindex] = TOUPPER (ftag[nmindex]);
> -
> -      flds = create_field (flds, substruct, "");
> -      flds->opt = create_option (nodot, "tag", ftag);
> +       flds = create_field (flds, substruct, "");
> +      flds->opt = create_string_option (nodot, "tag", ftag);
>      }
> -
> -  return new_structure ("rtx_def_subunion", 1, &lexer_line, flds, nodot);
> +   return new_structure ("rtx_def_subunion", 1, &lexer_line, flds, nodot);
>  }
> 
>  /* Handle `special("tree_exp")'.  This is a special case for
> 
> Indentation is wrong.
> 
>  }
> -
> -/* Set the gc_used field of T to LEVEL, and handle the types it references.  */
> -
> + /* Set the gc_used field of T to LEVEL, and handle the types it
> references.  */
>  static void
>  set_gc_used_type (type_p t, enum gc_used_enum level, type_p param[NUM_PARAM])
>  {
> 
> Comment should start on the first column.
> 
> -  d->needs_cast_p = false;
> +   d->needs_cast_p = false;
> 
> Please do not break indentation in the code you don't touch :)
> 
>    memset (&d, 0, sizeof (d));
>    d.of = get_output_file_for_structure (s, param);
> -
> -  for (opt = s->u.s.opt; opt; opt = opt->next)
> -    if (strcmp (opt->name, "chain_next") == 0)
> -      chain_next = opt->info;
> -    else if (strcmp (opt->name, "chain_prev") == 0)
> -      chain_prev = opt->info;
> -    else if (strcmp (opt->name, "chain_circular") == 0)
> -      chain_circular = opt->info;
> -    else if (strcmp (opt->name, "mark_hook") == 0)
> -      mark_hook_name = opt->info;
> -
> -  if (chain_prev != NULL && chain_next == NULL)
> +   for (opt = s->u.s.opt; opt; opt = opt->next)
> [...continues...]
> 
> Indentation is wrong.
> 
> +       for (o = v->opt; o; o = o->next)
> +	if (strcmp (o->name, "length") == 0
> +		       && o->kind == OPTION_STRING)
> +	  length = o->info.string;
> 
> Indentation is wrong.
> 
> +	else if (strcmp (o->name, "if_marked") == 0
> +		       && o->kind == OPTION_STRING)
> +	  if_marked = o->info.string;
> +       if (if_marked == NULL)
> 
> And wrong.
> 
> -
> -  /* We assemble the field list in reverse order.  */
> +   /* We assemble the field list in reverse order.  */
> 
> Again, you are not changing anything here except that you break indentation.
> 
> +	case OPTION_NONE:
> +	  fatal ("corrupted option %p: %s", (void*) o, o->name);
> 
> Insert default label too between these two lines.
> 
> +  TYPE_UNION,		/* Type for GTY-ed discriminated
> +			   union-s.  */
> +  TYPE_POINTER,		/* Pointer type to GTY-ed type.  */
> +  TYPE_ARRAY,		/* Array of GTY-ed types.  */
> 
> Align TYPE_POINTER comment.
> 
> +  OPTION_TYPE,			/* A type-valued option.  */
> +  OPTION_NESTED			/* Option data for 'nested_ptr'.  */
> 
> Align OPTION_NESTED comment.
> 
> +/* A way to pass data through to the output end.  */
> +struct options {
> +  struct options *next;		/* next option of the same pair.  */
> +  const char *name;		/* GTY option name.  */
> +  enum option_kind kind;		/* disciminating option kind.  */
> +  /* the union below is discriminated by the 'kind' field above.  */
> +  union {
> +    const char* string;		    /* When OPTION_STRING.  */
> +    type_p type;		    /* When OPTION_TYPE.  */
> +    struct nested_ptr_data* nested; /* when OPTION_NESTED.  */
> +  } info;
> +};
> 
> Align all the comments.


Regarding indentation issues in gengtype, please read
http://gcc.gnu.org/ml/gcc/2010-09/msg00448.html

I thanks warmly Laurynas for reading & commenting my patches, but I
need *specific* and *detailed* help on indentation, as detailed in the
message referenced above.  The best case for me is to be able to
submit a preliminary indentation patch, [option 1 of email
http://gcc.gnu.org/ml/gcc/2010-09/msg00448.html] which would only
change the indentation of gengtype.c & gengtype.h.  But I need
approval on the idea of submitting an indentation-only patch (this is
against GCC habits).

Only once indentation issues are settled would I be able to work on
the bulk of Laurynas comments. I put many hours of indentation efforts
in vain in these patches, and I strongly believe that it is because
the original (ie current trunk) gengtype.[ch] is badly indented, so I
don't know if I should perpetuate the indentation errors in gengtype
or correct them. (I did a mix of both).



Cheers.


-- 
Basile STARYNKEVITCH         http://starynkevitch.net/Basile/
email: basile<at>starynkevitch<dot>net mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mines, sont seulement les miennes} ***