Add CODING_STYLE document to standardize on some style issues.

Standardize brace handling around all else clauses according to new CODING_STYLE guidelines.

15 files changed, 297 insertions, 30 deletions

diff --git a/CODING_STYLE b/CODING_STYLE
new file mode 100644
index 000000000..c2c6d4a59
--- /dev/null
+++ b/CODING_STYLE
@@ -0,0 +1,211 @@
+Cairo coding style.
+
+This document is intended to be a short description of the preferred
+coding style for the cairo source code. Good style requires good
+taste, which means this can't all be reduced to automated rules, and
+there are exceptions.
+
+We want the code to be easy to understand and maintain, and consistent
+style plays an important part in that, even if some of the specific
+details seem trivial. If nothing else, this document gives a place to
+put consistent answers for issues that would otherwise be arbitrary.
+
+Most of the guidelines here are demonstrated by examples, (which means
+this document is quicker to read than it might appear given its
+length). Most of the examples are positive examples that you should
+imitate. The few negative examples are clearly marked with a comment
+of /* Yuck! */. Please don't submit code to cairo that looks like any
+of these.
+
+Indentation
+-----------
+Each new level is indented 4 more spaces than the previous level:
+
+	if (condition)
+	    do_something ();
+
+Spaces or tabs (or a combination) can be used in indentation, but tabs
+must always be interpreted as 8 spaces. Code using single tabs for all
+indentation (expecting people to interpret tabs as 4 spaces) will not
+be accepted in cairo.
+
+The rationale here is that tabs are used in the code for lining things
+up other than indentation, (see the Whitespace section below), and
+changing the interpretation of tab from 8 characters will break this.
+
+Braces
+------
+Most of the code in cairo uses bracing in the style of K&R:
+
+	if (condition) {
+	    do_this ();
+	    do_that ();
+	} else {
+	    do_the_other ();
+	}
+
+but some of the code uses an alternate style:
+
+	if (condition)
+	{
+	    do_this ();
+	    do_that ();
+	}
+	else
+	{
+	    do_the_other ();
+	}
+
+and that seems just fine. We won't lay down any strict rule on this
+point, (though there should be some local). If you came here hoping to
+find some guidance, then use the first form above.
+
+If all of the substatements of an if statement are single statements,
+the optional braces should not usually appear:
+
+	if (condition)
+	    do_this ();
+	else
+	    do_that ();
+
+But the braces are mandatory when mixing single statement and compund
+statements in the various clauses. For example, do not do this:
+
+	if (condition) {
+	    do_this ();
+	    do_that ();
+	} else			/* Yuck! */
+	    do_the_other ();
+
+And of course, there are exceptions for when the code just looks
+better with the braces:
+
+	if (condition) {
+	    /* Note that we have to be careful here. */
+	    do_something_dangerous (with_care);
+	}
+
+	if (condition &&
+	    other_condition &&
+	    yet_another)
+	{
+	    do_something ();
+	}
+
+And note that this last example also shows a situtation in which the
+opening brace really needs to be on its own line. The following looks awful:
+
+	if (condition &&
+	    other_condition &&
+	    yet_another) {	/* Yuck! */
+	    do_something ();
+	}
+
+As we said above, legible code that is easy to understand and maintain
+is the goal, not adherence to strict rules.
+
+Whitespace
+----------
+Separate logically distinct chunks with a single newline. This
+obviously applies between functions, but also applies within a
+function or block and can even be used to good effect within a
+structure definition:
+
+	struct _cairo_gstate {
+	    cairo_operator_t operator;
+    
+	    double tolerance;
+
+	    /* stroke style */
+	    double line_width;
+	    cairo_line_cap_t line_cap;
+	    cairo_line_join_t line_join;
+	    double miter_limit;
+
+	    cairo_fill_rule_t fill_rule;
+
+	    double *dash;
+	    int num_dashes;
+	    double dash_offset;
+	
+	    ...
+	}
+
+Use a single space before a left parenthesis, except where the
+standard will not allow it, (eg. when defining a parameterized macro).
+
+Don't eliminate whitespace just because things would still fit on one
+line. This breaks the expected visual structure of the code making it
+much harder to read and understand:
+
+	if (condition) foo (); else bar ();	/* Yuck! */
+
+As a special case of the bracing and whitespace guidelines, function
+definitions should always take the following form:
+
+	void
+	my_function (argument)
+	{
+	    do_my_things ();
+	}
+
+And function prototypes should similarly have the return type (and
+associated specifiers and qualifiers) on a line above the function, so
+that the function name is flush left.
+
+Break up long line (> ~80 characters) and use whitespace to align
+things nicely. For example the arguments in a long list to a function call should all be aligned with each other:
+
+	align_function_arguments (argument_the_first,
+				  argument_the_second,
+				  argument_the_third);
+
+And as a special rule, in a function prototype, (as well as in the
+definition), whitespace should be inserted between the parameter types
+and names so that the names are aligned:
+
+	void
+	align_parameter_names_in_prototypes (const char *char_star_arg,
+					     int	 int_arg,
+					     double	*double_star_arg,
+					     double	 duble_arg);
+
+Note that parameters with a * prefix are aligned one character to the
+left so that the actual names are aligned.
+
+Managing nested blocks
+----------------------
+Long blocks that are deeply nested make the code very hard to
+read. Fortunately such blocks often indicate logically distinct chunks
+of functionality that are begging to be split into their own
+functions. Please listen to the blocks when they beg.
+
+In other cases, gratuitous nesting comes about because the primary
+functionality gets buried in a nested block rather than living at the
+primary level where it belongs. Consider the following:
+
+	foo = malloc (sizeof (foo_t));
+	if (foo) {					/* Yuck! */
+	    ...
+	    /* lots of code to initialize foo */
+	    ...
+	    return SUCCESS;
+	}
+	return FAILURE;
+
+This kind of gratuitous nesting can be avoided by following a pattern
+of handling exceptional cases early and returning:
+
+	foo = malloc (sizeof (foo_t));
+	if (foo == NULL)
+	    return FAILURE;
+
+	...
+	/* lots of code to initialize foo */
+	...
+	return SUCCESS;
+
+The return statement is often the best thing to use in a pattern like
+this. If it's not available due to additional nesting above which
+require some cleanup after the current block, then consider splitting
+the current block into a new function before using goto.
diff --git a/ChangeLog b/ChangeLog
index e81fdac95..bf35f5b46 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,10 +1,29 @@
+2005-06-03  Carl Worth  <cworth@cworth.org>
+
+	* CODING_STYLE: Add CODING_STYLE document to standardize on some
+	style issues.
+	
+	* src/cairo-atsui-font.c:
+	* src/cairo-cache.c:
+	* src/cairo-ft-font.c:
+	* src/cairo-glitz-surface.c:
+	* src/cairo-gstate.c:
+	* src/cairo-matrix.c:
+	* src/cairo-pattern.c:
+	* src/cairo-pdf-surface.c:
+	* src/cairo-spline.c:
+	* src/cairo-wideint.c:
+	* src/cairo-win32-font.c:
+	* src/cairo-xlib-surface.c: Standardize brace handling around all
+	else clauses according to new CODING_STYLE guidelines.
+
 2005-06-03  Kristian Høgsberg  <krh@redhat.com>
 
 	Patch from Tomasz Cholewo <cholewo@ieee-cis.org>:
 
 	* src/cairo-pdf-surface.c: (cairo_pdf_ft_font_write_head_table),
 	(cairo_pdf_ft_font_generate): Store the index of the checksum
-	instea of a pointer to the location.
+	instead of a pointer to the location.
 
 2005-06-03  Carl Worth  <cworth@cworth.org>
 
diff --git a/src/cairo-atsui-font.c b/src/cairo-atsui-font.c
index a279956df..7460ca79f 100644
--- a/src/cairo-atsui-font.c
+++ b/src/cairo-atsui-font.c
@@ -486,14 +486,16 @@ _cairo_atsui_font_show_glyphs(void *abstract_font,
     CGContextSetTextMatrix(myBitmapContext, textTransform);
 
     if (pattern->type == CAIRO_PATTERN_SOLID &&
-	_cairo_pattern_is_opaque_solid(pattern)) {
+	_cairo_pattern_is_opaque_solid(pattern))
+    {
 	cairo_solid_pattern_t *solid = (cairo_solid_pattern_t *)pattern;
 	CGContextSetRGBFillColor(myBitmapContext,
 				 solid->color.red,
 				 solid->color.green,
 				 solid->color.blue, 1.0f);
-    } else
+    } else {
 	CGContextSetRGBFillColor(myBitmapContext, 0.0f, 0.0f, 0.0f, 0.0f);
+    }
 
     // TODO - bold and italic text
     //
diff --git a/src/cairo-cache.c b/src/cairo-cache.c
index e95894960..950f983b4 100644
--- a/src/cairo-cache.c
+++ b/src/cairo-cache.c
@@ -182,19 +182,25 @@ _cache_lookup (cairo_cache_t *cache,
 	{
 	    /* We are looking up an exact entry. */
 	    if (*probe == NULL)
+	    {
 		/* Found an empty spot, there can't be a match */
 		break;
+	    }
 	    else if (*probe != DEAD_ENTRY 
 		     && (*probe)->hashcode == hash
 		     && predicate (cache, key, *probe))
+	    {
 		return probe;
+	    }
 	}
 	else
 	{
 	    /* We are just looking for a free slot. */
 	    if (*probe == NULL 
 		|| *probe == DEAD_ENTRY)
+	    {
 		return probe;
+	    }
 	}
 
 	if (step == 0) { 	    
diff --git a/src/cairo-ft-font.c b/src/cairo-ft-font.c
index 0c1b5d78f..67a7891ee 100644
--- a/src/cairo-ft-font.c
+++ b/src/cairo-ft-font.c
@@ -575,10 +575,9 @@ _cairo_ft_unscaled_font_create_glyph (void                            *abstract_
     height = (unsigned int) ((cbox.yMax - cbox.yMin) >> 6);
     stride = (width + 3) & -4;
     
-    if (width * height == 0) 
+    if (width * height == 0) {
 	val->image = NULL;
-    else	
-    {
+    } else  {
 
 	bitmap.pixel_mode = ft_pixel_mode_grays;
 	bitmap.num_grays  = 256;
diff --git a/src/cairo-glitz-surface.c b/src/cairo-glitz-surface.c
index 65ed2f36e..d28bcdb52 100644
--- a/src/cairo-glitz-surface.c
+++ b/src/cairo-glitz-surface.c
@@ -1403,8 +1403,9 @@ _cairo_glitz_area_find (cairo_glitz_area_t *area,
 		return CAIRO_INT_STATUS_UNSUPPORTED;
 	
 	    _cairo_glitz_area_move_out (area);
-	} else
+	} else {
 	    return CAIRO_INT_STATUS_UNSUPPORTED;
+	}
 		
     /* fall-through */
     case CAIRO_GLITZ_AREA_AVAILABLE: {
@@ -1488,8 +1489,9 @@ _cairo_glitz_area_find (cairo_glitz_area_t *area,
 							 to_area->closure,
 							 closure) >= 0)
 		    return CAIRO_INT_STATUS_UNSUPPORTED;
-	    } else
+	    } else {
 		return CAIRO_INT_STATUS_UNSUPPORTED;
+	    }
 	}
 
 	for (i = 0; i < 4; i++)
diff --git a/src/cairo-gstate.c b/src/cairo-gstate.c
index 6b6a9e01b..c197b87ca 100644
--- a/src/cairo-gstate.c
+++ b/src/cairo-gstate.c
@@ -1079,9 +1079,13 @@ _clip_and_compute_extents_arbitrary (cairo_gstate_t    *gstate,
 	if (pixman_region_intersect (intersection,
 				     gstate->clip.region,
 				     intersection) == PIXMAN_REGION_STATUS_SUCCESS) 
+	{
 	    _region_rect_extents (intersection, extents);
+	}
 	else
+	{
 	    status = CAIRO_STATUS_NO_MEMORY;
+	}
 
 	pixman_region_destroy (intersection);
 
diff --git a/src/cairo-hash.c b/src/cairo-hash.c
index e95894960..950f983b4 100644
--- a/src/cairo-hash.c
+++ b/src/cairo-hash.c
@@ -182,19 +182,25 @@ _cache_lookup (cairo_cache_t *cache,
 	{
 	    /* We are looking up an exact entry. */
 	    if (*probe == NULL)
+	    {
 		/* Found an empty spot, there can't be a match */
 		break;
+	    }
 	    else if (*probe != DEAD_ENTRY 
 		     && (*probe)->hashcode == hash
 		     && predicate (cache, key, *probe))
+	    {
 		return probe;
+	    }
 	}
 	else
 	{
 	    /* We are just looking for a free slot. */
 	    if (*probe == NULL 
 		|| *probe == DEAD_ENTRY)
+	    {
 		return probe;
+	    }
 	}
 
 	if (step == 0) { 	    
diff --git a/src/cairo-matrix.c b/src/cairo-matrix.c
index 82ec0dbb7..733e86ad4 100644
--- a/src/cairo-matrix.c
+++ b/src/cairo-matrix.c
@@ -526,7 +526,9 @@ _cairo_matrix_compute_scale_factors (const cairo_matrix_t *matrix,
     _cairo_matrix_compute_determinant (matrix, &det);
 
     if (det == 0)
+    {
 	*sx = *sy = 0;
+    }
     else
     {
 	double x = x_major != 0;
diff --git a/src/cairo-pattern.c b/src/cairo-pattern.c
index 34b2cbe77..cad268629 100644
--- a/src/cairo-pattern.c
+++ b/src/cairo-pattern.c
@@ -868,8 +868,9 @@ _cairo_image_data_set_radial (cairo_radial_pattern_t *pattern,
 		    c0_x = y_x + c0_y;
 		    
 		    factor = (c0_e - r0) / (c0_x - r0);
-		} else
+		} else {
 		    factor = -r0;
+		}
 	    }
 
 	    _cairo_pattern_calc_color_at_pixel (&op, factor * 65536, pixels++);
@@ -1130,11 +1131,13 @@ _cairo_pattern_acquire_surface (cairo_pattern_t		   *pattern,
 							       attributes);
 	}
 	else
+	{
 	    status = _cairo_pattern_acquire_surface_for_gradient (src, dst,
 								  x, y,
 								  width, height,
 								  surface_out,
 								  attributes);
+	}
     } break;
     case CAIRO_PATTERN_SURFACE: {
 	cairo_surface_pattern_t *src = (cairo_surface_pattern_t *) pattern;
@@ -1169,8 +1172,11 @@ _cairo_pattern_release_surface (cairo_surface_t		   *dst,
 	_cairo_surface_release_source_image (dst,
 					     (cairo_image_surface_t *) surface,
 					     attributes->extra);
-    } else
+    }
+    else
+    {
 	cairo_surface_destroy (surface);
+    }
 }
 
 cairo_int_status_t
@@ -1199,7 +1205,7 @@ _cairo_pattern_acquire_surfaces (cairo_pattern_t	    *src,
      * support RENDER-style 4-channel masks. */
     if (src->type == CAIRO_PATTERN_SOLID &&
 	mask && mask->type == CAIRO_PATTERN_SOLID)
-      {
+    {
 	cairo_color_t combined;
 	cairo_solid_pattern_t *src_solid = (cairo_solid_pattern_t *) src;
 	cairo_solid_pattern_t *mask_solid = (cairo_solid_pattern_t *) mask;
@@ -1210,7 +1216,9 @@ _cairo_pattern_acquire_surfaces (cairo_pattern_t	    *src,
 	_cairo_pattern_init_solid (&tmp.solid, &combined);
 
 	mask = NULL;
-    } else {
+    }
+    else
+    {
 	_cairo_pattern_init_copy (&tmp.base, src);
     }
 
diff --git a/src/cairo-pdf-surface.c b/src/cairo-pdf-surface.c
index 76d1f6875..f1c47edb1 100644
--- a/src/cairo-pdf-surface.c
+++ b/src/cairo-pdf-surface.c
@@ -505,8 +505,7 @@ cairo_pdf_ft_font_write_glyf_table (cairo_pdf_ft_font_t *font,
 	if (header->Index_To_Loc_Format == 0) {
 	    begin = be16_to_cpu (u.short_offsets[index]) * 2;
 	    end = be16_to_cpu (u.short_offsets[index + 1]) * 2;
-	}
-	else {
+	} else {
 	    begin = be32_to_cpu (u.long_offsets[index]);
 	    end = be32_to_cpu (u.long_offsets[index + 1]);
 	}
@@ -631,8 +630,7 @@ cairo_pdf_ft_font_write_loca_table (cairo_pdf_ft_font_t *font,
     if (header->Index_To_Loc_Format == 0) {
 	for (i = 0; i < font->base.num_glyphs + 1; i++)
 	    cairo_pdf_ft_font_write_be16 (font, font->glyphs[i].location / 2);
-    }
-    else {
+    } else {
 	for (i = 0; i < font->base.num_glyphs + 1; i++)
 	    cairo_pdf_ft_font_write_be32 (font, font->glyphs[i].location);
     }
diff --git a/src/cairo-spline.c b/src/cairo-spline.c
index 5119a8e2b..60ad6c54d 100644
--- a/src/cairo-spline.c
+++ b/src/cairo-spline.c
@@ -64,23 +64,21 @@ _cairo_spline_init (cairo_spline_t *spline,
     spline->c = *c;
     spline->d = *d;
 
-    if (a->x != b->x || a->y != b->y) {
+    if (a->x != b->x || a->y != b->y)
 	_cairo_slope_init (&spline->initial_slope, &spline->a, &spline->b);
-    } else if (a->x != c->x || a->y != c->y) {
+    else if (a->x != c->x || a->y != c->y)
 	_cairo_slope_init (&spline->initial_slope, &spline->a, &spline->c);
-    } else if (a->x != d->x || a->y != d->y) {
+    else if (a->x != d->x || a->y != d->y)
 	_cairo_slope_init (&spline->initial_slope, &spline->a, &spline->d);
-    } else {
+    else
 	return CAIRO_INT_STATUS_DEGENERATE;
-    }
 
-    if (c->x != d->x || c->y != d->y) {
+    if (c->x != d->x || c->y != d->y)
 	_cairo_slope_init (&spline->final_slope, &spline->c, &spline->d);
-    } else if (b->x != d->x || b->y != d->y) {
+    else if (b->x != d->x || b->y != d->y)
 	_cairo_slope_init (&spline->final_slope, &spline->b, &spline->d);
-    } else {
+    else
 	_cairo_slope_init (&spline->final_slope, &spline->a, &spline->d);
-    }
 
     spline->num_points = 0;
     spline->points_size = 0;
diff --git a/src/cairo-wideint.c b/src/cairo-wideint.c
index 953108339..60946d90b 100644
--- a/src/cairo-wideint.c
+++ b/src/cairo-wideint.c
@@ -1,5 +1,5 @@
 /*
- * $Id: cairo-wideint.c,v 1.4 2005-01-19 15:07:00 cworth Exp $
+ * $Id: cairo-wideint.c,v 1.5 2005-06-03 21:51:57 cworth Exp $
  *
  * Copyright © 2004 Keith Packard
  *
@@ -421,7 +421,9 @@ _cairo_uint64_divrem (cairo_uint64_t num, cairo_uint64_t den)
 		    num = _cairo_uint64_sub (num, den);
 		}
 		else
+		{
 		    q0 = 0;
+		}
 		
 		q1 = 0;
 		r0 = num.lo;
@@ -937,7 +939,9 @@ _cairo_uint128_divrem (cairo_uint128_t num, cairo_uint128_t den)
 		    num = _cairo_uint128_sub (num, den);
 		}
 		else
+		{
 		    q0 = _cairo_uint32_to_uint64 (0);
+		}
 		
 		q1 = _cairo_uint32_to_uint64 (0);
 		r0 = num.lo;
diff --git a/src/cairo-win32-font.c b/src/cairo-win32-font.c
index 37e73022f..4d540d7f5 100644
--- a/src/cairo-win32-font.c
+++ b/src/cairo-win32-font.c
@@ -194,8 +194,9 @@ _get_system_quality (void)
 	}
 
 	return ANTIALIASED_QUALITY;
-    } else
+    } else {
 	return DEFAULT_QUALITY;
+    }
 }
 
 static cairo_scaled_font_t *
diff --git a/src/cairo-xlib-surface.c b/src/cairo-xlib-surface.c
index aa7679c8d..7564e9932 100644
--- a/src/cairo-xlib-surface.c
+++ b/src/cairo-xlib-surface.c
@@ -1122,13 +1122,14 @@ _cairo_xlib_surface_create_internal (Display		       *dpy,
 
     if (CAIRO_SURFACE_RENDER_HAS_CREATE_PICTURE (surface)) {
 	if (!format) {
-	    if (visual) {
+	    if (visual)
 		format = XRenderFindVisualFormat (dpy, visual);
-	    } else if (depth == 1)
+	    else if (depth == 1)
 		format = XRenderFindStandardFormat (dpy, PictStandardA1);
 	}
-    } else
+    } else {
 	format = NULL;
+    }
 
     surface->visual = visual;
     surface->format = format;
@@ -1778,20 +1779,26 @@ _cairo_xlib_surface_show_glyphs (cairo_scaled_font_t    *scaled_font,
 
     _cairo_xlib_surface_ensure_dst_picture (self);
     if (elt_size == 8)
+    {
 	status = _cairo_xlib_surface_show_glyphs8 (scaled_font, operator, g, &key, src, self,
 						   source_x + attributes.x_offset,
 						   source_y + attributes.y_offset, 
 						   glyphs, entries, num_glyphs);
+    }
     else if (elt_size == 16)
+    {
 	status = _cairo_xlib_surface_show_glyphs16 (scaled_font, operator, g, &key, src, self,
 						    source_x + attributes.x_offset,
 						    source_y + attributes.y_offset, 
 						    glyphs, entries, num_glyphs);
+    }
     else 
+    {
 	status = _cairo_xlib_surface_show_glyphs32 (scaled_font, operator, g, &key, src, self,
 						    source_x + attributes.x_offset,
 						    source_y + attributes.y_offset, 
 						    glyphs, entries, num_glyphs);
+    }
 
     for (i = 0; i < num_glyphs; ++i)
 	_xlib_glyphset_cache_destroy_entry (g, entries[i]);