From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8052 invoked by alias); 23 Sep 2010 14:08:06 -0000 Received: (qmail 8040 invoked by uid 22791); 23 Sep 2010 14:08:05 -0000 X-SWARE-Spam-Status: No, hits=-0.3 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,RCVD_IN_RP_RNBL X-Spam-Check-By: sourceware.org Received: from smtp-154-thursday.nerim.net (HELO maiev.nerim.net) (194.79.134.154) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 23 Sep 2010 14:07:59 +0000 Received: from hector.lesours (ours.starynkevitch.net [213.41.244.95]) by maiev.nerim.net (Postfix) with ESMTPS id A8AF32E05B; Thu, 23 Sep 2010 16:07:56 +0200 (CEST) Received: from basile18 by hector.lesours with local (Exim 4.72) (envelope-from ) id 1OymSq-00066t-71; Thu, 23 Sep 2010 16:07:56 +0200 Date: Thu, 23 Sep 2010 19:17:00 -0000 From: Basile Starynkevitch To: Laurynas Biveinis , gcc-patches@gcc.gnu.org Subject: Re: gengtype improvements for plugins, thirdround! patch 5/7 [typedopt] Message-ID: <20100923140756.GB20811@hector.lesours> References: <20100921210301.d92889be.basile@starynkevitch.net> <20100921210829.5f2ea8b0.basile@starynkevitch.net> <20100921211217.df1ef337.basile@starynkevitch.net> <20100921211628.969ed469.basile@starynkevitch.net> <20100921212431.f07dad12.basile@starynkevitch.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.20 (2009-06-14) X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2010-09/txt/msg01898.txt.bz2 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 : > > @@ -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: basilestarynkevitchnet mobile: +33 6 8501 2359 8, rue de la Faiencerie, 92340 Bourg La Reine, France *** opinions {are only mines, sont seulement les miennes} ***