Justification patch, style issues

I've started looking through the justification patch, and well, I
certainly haven't figured it all out yet. But I wanted to give some
feedback on stylistic issues in the patch.

Some general nits:

 - Pango doesn't gint, but rather int, and so forth.

 - Comments have a line of '*' down the left line.
   /* Blah blah blah [...]
    * blah

   The trailing */ goes on a separate line, except for single line
   comments or when the comment applies to a single line of code.
   (A comment preceding a function should have the */ on a separate
   line even if it is a single line)

 - Operators should be broken on the preceding line, not the 
   next line.

   if (foo ||

   I don't care for the GNU-recommended:

   if (foo
       || bar)

 - In general, there should be a line of white space between variable
   declarations and code.

   for (l = items; l; l = l->next) 
       PangoItem *item = l->data;
       /* do something with item */

 - Multiple declarations on the same line should only be used when
   the elements are closely related.

   GList *start_node, *end_node;
   int x, y;

   Are OK.

   int last_tab_byte_offset = -1, i, j;

   is not.

 - GList variables should be named 'l', not 'elem'. (We used to use
   tmp_list but that was clunky.) If you find you need two such
   variables at once, the code probably needs division into separate
   functions. If not, use a descriptive name like 'item_node'.

 - I'd suggest always using double, and never float. I'd only
   use floats in cases where space saving is a big issue.

Then turning to a few things I saw in the patch that I had difficulty
with stylistically:

/* This justifies the line of text, using the settings calculated in
   finish_line(). */
static void
justify_line (PangoLayout      *layout,
             GSList            *runs,
             LineInfo          *line_info,
             JustificationInfo *justification_info,
             PangoLayoutRun    *line_start_run,
             PangoLayoutRun    *line_end_run,
             gint              *line_start_adjustment,
             gint              *line_end_adjustment)

We have 8 parameters here, some of with return values, some not, with no
documentation of what they do. I'd really like to keep functions to no
more than about 5 parameters. (Less is better.) If more are needed,
that almost certainly means that some sort of structure is needed.

If a functions parameters are non-obvious they should be documented;
that can be done informally; though I occasionally resort to gtk-doc
style comments even for static functions. (Internally exported functions
that are used across file boundaries always get doc comments)

Then we come to layout_with_specific_width. To quote the local variables
defined in it:

  GArray *items_data, single_items_data;
  ItemData *item_data, single_item_data;
  LineInfo line_info = { 0 };
  PangoItem *item;
  const gchar *byte_pos = NULL, *line_end_byte_pos = NULL;
  PangoGlyphUnit required_line_width, saved_line_width, natural_line_width;
  PangoGlyphUnit total_distance = 0, break_distance = 0;
  PangoGlyphUnit max_compression = 0, log_width, min_letter_spacing;
  PangoGlyphUnit best_break_natural_line_width = 0;
  PangoGlyphUnit total_compressible_width = 0, break_compressible_width = 0;
  PangoLogAttr log_attr;
  gboolean use_line_breaks = TRUE, use_char_breaks = TRUE;
  gboolean trim_overflow = FALSE, overflowed = FALSE, need_full_copy = FALSE;
  gint line_start_item_index = 0, item_index = 0;
  gint char_offset, line_end_char_offset;
  gint log_widths_char_num = 0, char_num = 0;
  gint is_line_break = 0, is_char_break = 0, break_score;
  gint last_tab_byte_offset = -1, i, j;
  gint overflow_byte_offset = -1, overflow_char_offset = -1;
  /* These are the total widths of glyphs, resizable space and number f
     clusters up to the current position, the preferred break position, nd
     the point we overflowed the line. (See the comments on TOTAL, O_BREAK
     and OVERFLOW above.) */
  PangoGlyphUnit total_width[NUM_SCRIPT_GROUPS][3];
  PangoGlyphUnit resizable_width[NUM_SCRIPT_GROUPS][3];
  gint num_clusters[NUM_SCRIPT_GROUPS][3];

  /* This is the information we keep for the best break found so far. */
  gint best_break_item_index = -1, best_break_score = G_MAXINT;
  gboolean best_break_is_line_break = FALSE;
  gint best_break_log_widths_char_num = -1;

Obviously this is doing something really hard and complicated. But we
need to figure out how to split up 421 line function with 44 local

The approach I've taken elsewhere - ellipsize.c, pango_itemize(), the
old line break code, was to use a structure to hold the state of the
operation - at that point, breaking stuff down into identifiable
functions becomes easier. And we also separate out the variables that
*aren't* state (i, j, say), from the state. The structure also provides
a convenient place to document the tricky part of the state.

justify_line() probably could also use some splitting up.

Finally, I would like to see this in a separate file. pango-layout.c is
pushing the bounds of maintainability already; I don't know how much
this actually adds, but it looks like between 1000 and 2000 lines.

I don't like asking you to do more work on this for purely stylistic
reasons, since you've already spent so much time just getting it
working, but I do think it is important to keep the code as readable
and maintainable as possible.


Attachment: signature.asc
Description: This is a digitally signed message part

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]