This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

Re: Patch for automaton based pipeline hazard recognizer (part #3).


On Wed, 31 Jan 2001, Vladimir Makarov wrote:
>   size_t curr_index;
[...]
>         for (curr_index = 0;
>              curr_index < VLA_PTR_LENGTH (first_insns_with_same_reservs);
>              curr_index++)

Sometimes, a few variable names are a bit verbose.  "i" is perfectly OK as
a loop counter; using "i" may avoid some of the formatting problems - lines
don't wrap around so easily.

I'd also find this slightly more readable as
	size_t length = VLA_PTR_LENGTH (first_insns_with_same_reservs);
	for (i = 0; i < length; i++)

> /* The function transformes nondeterminstic AUTOMATON into
>    determenistic. */

"deterministic".  Two spaces before end of comment.

>       for (curr_state_ptr = VLA_PTR_BEGIN (all_achieved_states);
> 	   curr_state_ptr
> 	     <= (struct state **) VLA_PTR_LAST (all_achieved_states);
>            curr_state_ptr++)
> 	if (odd_iteration_flag)
> 	  (*curr_state_ptr)->equiv_class_num_2
> 	    = (*curr_state_ptr)->equiv_class_num_1;
> 	else
> 	  (*curr_state_ptr)->equiv_class_num_1
> 	    = (*curr_state_ptr)->equiv_class_num_2;

I believe shorter variable names would help that code as well (and
I'd also argue that instead of equiv_class_num_1, equiv_class_1 would
be just as readable and less likely to cause line wrap).
(This sort of thing is common throughout, I'm picking out examples).

>                           /* Remove curr state from the class
>                              equivalence. */
>                           prev_state->next_equiv_class_state = next_state;
>                           /* Add curr state to the new class equivalence. */
>                           curr_state->next_equiv_class_state = new_equiv_class;
>                           if (new_equiv_class == NULL)
>                             new_equiv_class_num++;
>                           if (odd_iteration_flag)
>                             curr_state->equiv_class_num_2
>                               = new_equiv_class_num;
>                           else
>                             curr_state->equiv_class_num_1
>                               = new_equiv_class_num;
>                           new_equiv_class = curr_state;
>                           finish_flag = FALSE;

Likewise here, although at this level of indentation breaking up the
function may be the best choice.

>   for (curr_equiv_class_ptr = VLA_PTR_BEGIN (*equiv_classes);
>        curr_equiv_class_ptr <= (struct state **) VLA_PTR_LAST (*equiv_classes);
>        curr_equiv_class_ptr++)
>     if ((*curr_equiv_class_ptr)->next_equiv_class_state != NULL)
>       {
>         first_class_state = *curr_equiv_class_ptr;
>         /* Create new arcs output from the state corresponding to
>            equiv class. */
>         for (curr_arc = first_out_arc (first_class_state);
>              curr_arc != NULL;
>              curr_arc = next_out_arc (curr_arc))
>           add_arc (first_class_state->equiv_class_state,
>                    curr_arc->to_state->equiv_class_state,
> 		   curr_arc->insn, curr_arc->state_alts);
>         /* Delete output arcs from states of given class equivalence. */
>         for (curr_state = first_class_state;
>              curr_state != NULL;
>              curr_state = curr_state->next_equiv_class_state)
>           {
>             if (automaton->start_state == curr_state)
>               automaton->start_state = curr_state->equiv_class_state;
>             /* Delete the state and its output arcs. */
>             for (curr_arc = first_out_arc (curr_state);
>                  curr_arc != NULL;
>                  curr_arc = next_arc)
>               {
>                 next_arc = next_out_arc (curr_arc);
>                 free_arc (curr_arc);
>               }
>           }
>       }

More whitespace would help - e.g. empty lines in front of each comment.
(This is also quite common - ideally there should be a line of whitespace
whenever two conceptually separate pieces of code are adjacent).

> /* Name of the PHR interface macro. */

> #define I_VARIABLE_NAME "i"

[... and many other macros that define strings ...]

The comments I made in the previous email still apply.  This is completely
unnecessary clutter IMO.

>      void (*output_full_vect_name_func)
>        PARAMS ((FILE *f, struct automaton *automaton));
>      void (*output_comb_vect_name_func)
>        PARAMS ((FILE *f, struct automaton *automaton));
>      void (*output_check_vect_name_func)
>        PARAMS ((FILE *f, struct automaton *automaton));
>      void (*output_base_vect_name_func)
>        PARAMS ((FILE *f, struct automaton *automaton));

Maybe add a typedef.

>
> static void
> add_vect (tab, vect_num, vect, vect_length)
>      struct state_ainsn_table *tab;
>      int vect_num;
>      vect_el_t *vect;
>      int vect_length;

Function has no comment.


> /* Output function `internal_insn_latency'. */

I mentioned this before, but there's no documentation for what these
generated functions do.


Bernd


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]