Re: coding style, continuing education

["Zack Weinberg" <zackw at stanford dot edu>]

On Mon, Jan 15, 2001 at 02:03:33PM -0800, Mike Stump wrote:
> I know that historically gcc has had thousand line functions, and
> worse, but as time goes on we clean up the various sorts of code and
> make it prettier.  It is just like the line formatting rules, see a
> line longer than 80, and one should just split on demand, not stopping
> to think twice about if they should do it or not.  I'm tracking down a
> problem on an older compiler, and was traipsing around the code below.
> Icky.  I think it would help if people realize that this code is like
> the > 80 character lines, one should just take a moment and split it
> down.

I like the general principle, but the trouble is that unlike
line-splitting, it can be excruciatingly difficult to be certain you
haven't changed the meaning of the code.  I've a patch on ice for
function.c, which was _supposed_ to have no semantic effect, but
caused ~500 testsuite regressions...

Doesn't help when you can't tell what the 50-line if clause is
_supposed_ to be testing.

> 	if (mode1 == VOIDmode
> 	    || GET_CODE (op0) == REG || GET_CODE (op0) == SUBREG
> 	    || (modifier != EXPAND_CONST_ADDRESS
> 		&& modifier != EXPAND_INITIALIZER
> 		&& ((mode1 != BLKmode && ! direct_load[(int) mode1]
> 		     && GET_MODE_CLASS (mode) != MODE_COMPLEX_INT
> 		     && GET_MODE_CLASS (mode) != MODE_COMPLEX_FLOAT)
> 		    /* If the field isn't aligned enough to fetch as a memref,
> 		       fetch it as a bit field.  */
> 		    || (mode1 != BLKmode
> 			&& SLOW_UNALIGNED_ACCESS (mode1, alignment)
> 			&& ((TYPE_ALIGN (TREE_TYPE (tem))
> 			     < GET_MODE_ALIGNMENT (mode))
> 			    || (bitpos % GET_MODE_ALIGNMENT (mode) != 0)))
> 		    /* If the type and the field are a constant size and the
> 		       size of the type isn't the same size as the bitfield,
> 		       we must use bitfield operations.  */
> 		    || ((bitsize >= 0
> 			 && (TREE_CODE (TYPE_SIZE (TREE_TYPE (exp)))
> 			     == INTEGER_CST)
> 			 && 0 != compare_tree_int (TYPE_SIZE (TREE_TYPE (exp)),
> 						   bitsize)))))
> 	    || (modifier != EXPAND_CONST_ADDRESS
> 		&& modifier != EXPAND_INITIALIZER
> 		&& mode == BLKmode
> 		&& SLOW_UNALIGNED_ACCESS (mode, alignment)
> 		&& (TYPE_ALIGN (type) > alignment
> 		    || bitpos % TYPE_ALIGN (type) != 0)))
> 	  {

This looks like it badly wants some abstract macros.  For instance,

> 		&& ((mode1 != BLKmode && ! direct_load[(int) mode1]
> 		     && GET_MODE_CLASS (mode) != MODE_COMPLEX_INT
> 		     && GET_MODE_CLASS (mode) != MODE_COMPLEX_FLOAT)

		&& ((INDIRECT_LOAD_NON_BLKMODE (mode1)
		     && NON_COMPLEX_MODE (mode))

which is only one line shorter, but a fair bit clearer IMO.

zw