RE: coding style, continuing education

RE: coding style, continuing education

[David Korn]

>-----Original Message-----
>From: Michael Matz [ mailto:matzmich@cs.tu-berlin.de ]
>Sent: 17 January 2001 17:40

>On Mon, 15 Jan 2001, Mike Stump wrote:
>> 
>> If people _know_ of a good way to split this down, maybe we all just
>> need a refresher course on the right way, care to elaborate?
>> 
>> [ long expression snipped ]
>
>Even more entertaining is reload.c:push_reload. It has two expressions (in
>if's), 62 and 50 lines long, intersparsed with #ifdef's, so it not even
>fits on one editor page. They have comments in front of them, but it isn't
>exactly easy to at least verify, if these expressions actually do, what
>the comments explain ;-)

  You know what would come in handy here?  Some kind of automated tool for
processing these messes.  I imagine it would have to read in and parse a
bit of C code, building a syntax tree as it went.  It could even simplify
the expression somewhat by spotting duplicated subexpressions and 
eliminating them, or by applying mathematical transformations and boolean
algebra.  Obviously it would have to know all about C types and evaulation
rules, but hopefully it could then output the transformed code in some
simpler format.

  Oh.  Hold on a minute.  We already have one of those...

         DaveK
-- 
The Boulder Pledge: "Under no circumstances will I ever purchase anything 
offered to me as the result of an unsolicited email message. Nor will I 
forward chain letters, petitions, mass mailings, or virus warnings to large 
numbers of others. This is my contribution to the survival of the online
community."


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

This footnote also confirms that this email message has been swept by
MIMEsweeper for the presence of computer viruses.

www.mimesweeper.com
**********************************************************************

Re: coding style, continuing education

[Bruce Korb]

Nix wrote:

> So we need a way to format them nicely. Might I throw out the
> possibility of something like

If it is too complicated to understand without interspersed
comments, then it is too complicated, period.  *PLEASE* do
not show off your prowess at minimizing expression sizes.
That is a compiler problem.  Minimize brain comprehension
cycles by keeping your focus narrow.  That is the maintenance
problem.  Cascading `if's are (usually) *far* easier than a
multi-leveled gargantuan monstrosity.

Re: coding style, continuing education

[Nix]

On Fri, 19 Jan 2001, Phil Edwards spake:
> Chapter 4 starts off:
> 
> #   Functions should be short and sweet, and do just one thing.  They should
> #   fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
> #   as we all know), and do one thing and do that well.
> 
> The 60-line 'if' expressions pretty much blew that test.

The problem with *that* suggestion is that, for some of those
expressions, the only way to re-express them is either to produce a
bunch of one-off macros (which is quite ugly) or to cut the if statement
into a ladder of ifs (which is simply pointless).

I think that particular suggestion is all well and good for a kernel ---
which, after all, hasn't got *that* many desperately complicated
decisions to make (many of its decisions being made for it, at compile
time) --- but may not be so useful for a compiler, the whole raison d'etre
of which is to indulge in desperately complicated decisions...

I don't think hugely long conditional expressions should be common, by
any means, but I don't think that if they occur and can't be decomposed
easily[1], the code should be distorted to fit this sort of rule.

So we need a way to format them nicely. Might I throw out the
possibility of something like

    <code>

    /* Verify if we need to... */

        /* We need to if... */
    if (((...)
        && (...)
        && ((...)
           || (...)))
        /* ... or if... */
        || (...)

i.e. indent the comments within the conditional expression to the
nesting level of the operators that precede the brackets in that part of
the nesting.

(Maybe it looks too ugly. I'm not sure. Certainly it makes the
distinction between the block comment and the smaller comments above
conditional pieces more concrete.)


btw, I just noticed; at least *some* long conditionals, like the one I
pointed at in combine, are already using this style; at least the
indentation of the comments part. The combine conditional simply
mis-indents the topmost comment and doesn't have a description of the
entire conditional.

The GNU coding standards don't seem to comment on this.

> Of course, this is also the file that starts off by recommending that the
> prospective coder print out a copy of the GNU coding standards and burn
> them as a symbolic gesture.  :-)  Which I must admit I've been tempted to
> do on many occasions, but that's not germane to the discussion...

Oddly, I found that in ~87--90 I independently evolved a coding style
that's almost exactly like the GNU coding standards. I'd never heard of
GNU (or even free software) at the time, and had never seen any GNU
source; perhaps this was morphic resonance ;)

Seeing GNU-style source, in the morass of K&R-style indentation and
Whitesmiths indentation out there is like coming home...


[1] i.e. separating out generic and generally useful tests into macros,
    rather than separating out specialized ones just to cut the
    conditional size down (not that we'd do that; but I've seen it done
    in other code, and it's not nice.)

-- 
`Anyhow, that pipe dream doesn't say anything about the question you
 asked.  (I am planning for a career in politics.)' --- Mark Mitchell
                                                      on the GCC list

Re: coding style, continuing education

[McNulty Junior Bobby]

Hey, quit talking about my coding style. :)
If you ever seen seen son of mine before i had a
friend clean it up, then you'd see why I quit coding.



--- Joerg Faschingbauer <jfasch@aon.at> wrote:
> From: dewar@gnat.com
> Subject: Re: coding style, continuing education
> Date: Mon, 15 Jan 2001 19:44:25 -0500 (EST)
> 
> > <<Caution -- sequence points, lazy evaluation,
> side effects. It's not just
> > boolean algebra.
> > 
> > Tom
> > >>
> > 
> > If this makes a difference, you are talking about
> seriously horrible code :-)
> 
> Bah! What about this one :-?
> 
>    /* :-))) */
>    (
>       // the basic things.
>       (e += typespec_.matchExact (p.type())).error()
> || 
>       (e += namespec_.match (p.name())).error() ||
> 
>       // either both enums are set or none.
>       (enumspec_.isSet() && !p.eNum().isSet() ||
> !enumspec_.isSet() && p.eNum().isSet()) && 
>       (e += HW_Reason(classid,E_MATCH), 1) ||
> 
>       // either both ranges are set or none.
>       (rangespec_.isSet() && !p.range().isSet() ||
> !rangespec_.isSet() && p.range().isSet()) && 
>       (e += HW_Reason(classid,E_MATCH), 1) ||
>       
>       // enum and range (if given) must match
> exactly.
>       enumspec_.isSet() && (e += enumspec_.match
> (p.eNum())).error() ||
>       rangespec_.isSet() && (e += rangespec_.match
> (p.range())).error() ||
>       
>       // either .. or ...
>       (defaultspec_.isSet() && !p.def().isSet() ||
> !defaultspec_.isSet() && p.def().isSet()) &&
>       (e += HW_Reason(classid,E_MATCH), 1) ||
> 
>       (defaultspec_.isSet() && 
>        ((((e += defaultspec_.value().equal(b,
> p.def().value())).error()) || !b) && 
>           (e += HW_Reason(classid,E_MATCH), 1)))
>       ) ;
> 
> Joerg


=====
Robert McNulty Junior
Robert McNulty Junior (C) 2000
email: bmcnultyjunior@yahoo.com
Homepage: http://www.geocities.com/bmcnultyjunior
AIM:bmcnultyjrsap
Featuring Csound compiled with a Win32/Intel Based Compiler

Re: coding style, continuing education

[Phil Edwards]

On Fri, Jan 19, 2001 at 05:46:33AM -0500, Richard Kenner wrote:
>     with a thoroughly misleading comment above it (is that what the entire
>     conditional does? no, it's what the first clause does), 
> 
> That raises an interesting issue: where *does* the comment for what the first
> clause does go?  I've been putting it before the entire statement, but,
> as you point out, that's not ideal.  Should we be doing something like:
> 
> 	/* This tells when we need to ....  */
> 
> 	if (
> 	    /* We need to if ... */
> 	    <conditional expression>
> 	    /* ... or if ... */
>             <next conditional expression>

That's pretty much the style I use; as Tom says, the pre-function comment
is the most important one.

The Linux coding style file has some interesting comments (no pun) to make:

#   Also, try to avoid putting comments inside a function body: if the
#   function is so complex that you need to separately comment parts of it,
#   you should probably go back to chapter 4 for a while.  You can make small
#   comments to note or warn about something particularly clever (or ugly),
#   but try to avoid excess.  Instead, put the comments at the head of the
#   function, telling people what it does, and possibly WHY it does it.

Chapter 4 starts off:

#   Functions should be short and sweet, and do just one thing.  They should
#   fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
#   as we all know), and do one thing and do that well.

The 60-line 'if' expressions pretty much blew that test.

Of course, this is also the file that starts off by recommending that the
prospective coder print out a copy of the GNU coding standards and burn
them as a symbolic gesture.  :-)  Which I must admit I've been tempted to
do on many occasions, but that's not germane to the discussion...


-- 
pedwards at disaster dot jaj dot com  |  pme at sources dot redhat dot com
devphil at several other less interesting addresses in various dot domains
The gods do not protect fools.  Fools are protected by more capable fools.

Re: coding style, continuing education

[Tom Leete]

Richard Kenner wrote:
> 
>     with a thoroughly misleading comment above it (is that what the entire
>     conditional does? no, it's what the first clause does),
> 
> That raises an interesting issue: where *does* the comment for what the first
> clause does go?  I've been putting it before the entire statement, but,
> as you point out, that's not ideal.  Should we be doing something like:
> 
>         /* This tells when we need to ....  */
> 
>         if (
>             /* We need to if ... */
>             <conditional expression>
>             /* ... or if ... */
>             <next conditional expression>

I think the comment before the entire statement is the most important. It
should, IMO, document the relation between the conditional and the
consequent; not just the meaning of the conditional. If a short abstract
description of the conditional cannot be produced to justify the entire
statement, then words to the effect of "some conditions require that we..."
should be used, with further comments for each independent clause.

In a nutshell: Don't comment to interpret the language for novices, always
say why the consequent block must be done given the condition.

Tom

Re: coding style, continuing education

[Richard Kenner]

    with a thoroughly misleading comment above it (is that what the entire
    conditional does? no, it's what the first clause does), 

That raises an interesting issue: where *does* the comment for what the first
clause does go?  I've been putting it before the entire statement, but,
as you point out, that's not ideal.  Should we be doing something like:

	/* This tells when we need to ....  */

	if (
	    /* We need to if ... */
	    <conditional expression>
	    /* ... or if ... */
            <next conditional expression>

Re: coding style, continuing education

[Nix]

On 17 Jan 2001, Michael Matz said:
> Even more entertaining is reload.c:push_reload. It has two expressions (in
> if's), 62 and 50 lines long, intersparsed with #ifdef's, so it not even
> fits on one editor page. They have comments in front of them, but it isn't
> exactly easy to at least verify, if these expressions actually do, what
> the comments explain ;-)

My current favourite is in combine.c:can_combine_p(); 60 lines long,
with a thoroughly misleading comment above it (is that what the entire
conditional does? no, it's what the first clause does), and with a `#if
0' in it as well. At least the body is simple, and it's commented...

-- 
`Anyhow, that pipe dream doesn't say anything about the question you
 asked.  (I am planning for a career in politics.)' --- Mark Mitchell
                                                      on the GCC list

Re: coding style, continuing education

[Joerg Faschingbauer]

From: dewar@gnat.com
Subject: Re: coding style, continuing education
Date: Mon, 15 Jan 2001 19:44:25 -0500 (EST)

> <<Caution -- sequence points, lazy evaluation, side effects. It's not just
> boolean algebra.
> 
> Tom
> >>
> 
> If this makes a difference, you are talking about seriously horrible code :-)

Bah! What about this one :-?

   /* :-))) */
   (
      // the basic things.
      (e += typespec_.matchExact (p.type())).error() || 
      (e += namespec_.match (p.name())).error() ||

      // either both enums are set or none.
      (enumspec_.isSet() && !p.eNum().isSet() || !enumspec_.isSet() && p.eNum().isSet()) && 
      (e += HW_Reason(classid,E_MATCH), 1) ||

      // either both ranges are set or none.
      (rangespec_.isSet() && !p.range().isSet() || !rangespec_.isSet() && p.range().isSet()) && 
      (e += HW_Reason(classid,E_MATCH), 1) ||
      
      // enum and range (if given) must match exactly.
      enumspec_.isSet() && (e += enumspec_.match (p.eNum())).error() ||
      rangespec_.isSet() && (e += rangespec_.match (p.range())).error() ||
      
      // either .. or ...
      (defaultspec_.isSet() && !p.def().isSet() || !defaultspec_.isSet() && p.def().isSet()) &&
      (e += HW_Reason(classid,E_MATCH), 1) ||

      (defaultspec_.isSet() && 
       ((((e += defaultspec_.value().equal(b, p.def().value())).error()) || !b) && 
          (e += HW_Reason(classid,E_MATCH), 1)))
      ) ;

Joerg

Re: coding style, continuing education

[Michael Matz]

Hi,

On Mon, 15 Jan 2001, Mike Stump wrote:
> 
> If people _know_ of a good way to split this down, maybe we all just
> need a refresher course on the right way, care to elaborate?
> 
> [ long expression snipped ]

Even more entertaining is reload.c:push_reload. It has two expressions (in
if's), 62 and 50 lines long, intersparsed with #ifdef's, so it not even
fits on one editor page. They have comments in front of them, but it isn't
exactly easy to at least verify, if these expressions actually do, what
the comments explain ;-)


Ciao,
Michael.

Re: coding style, continuing education

[Bill Wendling]

Also sprach Mike Stump:
} 
} If people _know_ of a good way to split this down, maybe we all just
} need a refresher course on the right way, care to elaborate?
} 
} 
} 	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)))
} 	  {

FOr this code snippet in particular, a big honkin' comment right above it
which explains what it's doing would be nice. That is, if it can't be
recoded in a much nicer fashion.

My thoughts.

-- 
|| Bill Wendling			wendling@ncsa.uiuc.edu
|| Coding Simian

RE: coding style, continuing education

[Bruce Korb]

I would add one more KISS rule:

Never ever regard any other rules as absolute :-)

True.  I forgot that one  ;-)

Re: coding style, continuing education

[dewar]

I would add one more KISS rule:

Never ever regard any other rules as absolute :-)

Seriously, none of those 6 rules are absolute rules, and there can
be good reasons for breaking any of them!

Re: coding style, continuing education

[Bruce Korb]

sidster wrote:
> 
> Greetings,
> 
> How about this style:

>         if
>             (
>                 [[reformatted junk elided]]
>             )


NO!  (in my most unhumble opinion).

Logical KISS rules to live by:

1.  Never, ever, more than two logical levels
2.  If you do use two, then think long and hard about having
    5-8 terms in the expression.  More than 8?
    Then consider another line of work.
3.  If only one, then, perforce, the logical expressions are
    all primitives (boolean variables or boolean expressions)
    joined by either `&&' or `||', but not both.
4.  Parenthesize everything.  *EVERY*thing.  The goal is
    absolute clarity, not typing conservation.
5.  Vertically align expressions (you did this one, good! :)
6.  If some of the expressions have side effects (set values
    in addition to testing them), then BREAK IT UP!

*sigh* ...

Re: coding style, continuing education

[sidster]

Greetings,

How about this style:


/* --- begin paste --- */
//
//
//
// Pretticized and grouped
//
//    Note the "group_xxxxx" comments added.  They'll make sense
//    after you see the second attachment.
//

        if
            (
                mode1 == VOIDmode
            ||  GET_CODE (op0) == REG
            ||  GET_CODE (op0) == SUBREG
            ||  (   /* group_0 */
                    modifier != EXPAND_CONST_ADDRESS
                &&  modifier != EXPAND_INITIALIZER
                &&  (   /* group_0_0 */
                        (   /* group_0_1_0 */
                            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. */
                    ||  (   /* group_0_1_1 */
                            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. */
                    ||  (   /* group_0_1_2 */
                            (
                                bitsize >= 0
                            &&  (
                                    TREE_CODE (TYPE_SIZE (TREE_TYPE (exp)))
                                    == INTEGER_CST
                                )
                            &&  (
                                    0 != compare_tree_int
                                            (
                                                TYPE_SIZE (TREE_TYPE (exp)),
                                                bitsize
                                            )
                                )
                            )
                        )
                    )
                )
            ||  (   /* group_1 */
                    modifier != EXPAND_CONST_ADDRESS
                &&  modifier != EXPAND_INITIALIZER
                &&  mode == BLKmode
                &&  SLOW_UNALIGNED_ACCESS (mode, alignment)
                &&  (
                        TYPE_ALIGN (type) > alignment
                        || bitpos % TYPE_ALIGN (type) != 0
                    )
                )
            )
        {
            printf( "And yes... we are done!\n" );
        }

/* --- end paste --- */


The tabs are replaced with spaces since i never use tabs in my code
(tabs make printouts look like hell).  But of course you can replace
every 4 spaces w/a tab character ( in vi  :1,$s,   ,^I,g ) if it is
a religious thing.

Of course, now you'd want to write "shorthand"ed versions of some of
the grouped code above to even make it more compact and readable (if
possible).  [See attachments]


I'm attaching the source files:

   one that is just "pretticized.c" and
   the other "pretticized-and-broken-down.c"



Hope this helps and that the following was not a rhetorical questions:

> If people _know_ of a good way to split this down, maybe we all just
> need a refresher course on the right way, care to elaborate?


Otherwise, I'll feel like a total idiot spending time on this!  >:-)




* Mike Stump (mrs@windriver.com) [20010115 14:04]:
> 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 think we should all just agree that conditionals that run on
> past 20 or so lines should be split.  Maybe we need a new gcc warning
> when the opening ( and the closing ) are more than 20 lines apart, we
> warn.  We could also have a { } checker that will warn when they are
> more than, say 2000 lines apart.  Someone want to work on a useful
> warning that will hit often?  :-)
> 
> If people _know_ of a good way to split this down, maybe we all just
> need a refresher course on the right way, care to elaborate?
> 
> 
> 	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)))
> 	  {

patrick
--
It's a damn poor mind that can only think of one way to spell a word.
      -- Andrew Jackson

Re: coding style, continuing education

[Bruce Korb]

Tom Leete wrote:
> Caution -- sequence points, lazy evaluation, side effects. It's not just
> boolean algebra.

Anything that requires tedious, careful examination in order to decipher
the functionality is wrong and needs rewriting.  I was also being
flippant.
:-)

Re: coding style, continuing education

[dewar]

<<Caution -- sequence points, lazy evaluation, side effects. It's not just
boolean algebra.

Tom
>>

If this makes a difference, you are talking about seriously horrible code :-)

Re: coding style, continuing education

[Tom Leete]

Bruce Korb wrote:
> 
> Mike Stump wrote:
> 
> > If people _know_ of a good way to split this down, maybe we all just
> > need a refresher course on the right way, care to elaborate?
> 
> It's simple  :-)
> 
> Put the `then' clause into a routine, recast the expression using
> boolean algebra to reduce nesting and break it up into a series
> of `if' statements that each have a clear purpose.
> 
> >         if ( [[insufferable boolean expression elided]] )
> >           {

Caution -- sequence points, lazy evaluation, side effects. It's not just
boolean algebra.

Tom

Re: coding style, continuing education

[Bruce Korb]

Mike Stump wrote:

> If people _know_ of a good way to split this down, maybe we all just
> need a refresher course on the right way, care to elaborate?

It's simple  :-)

Put the `then' clause into a routine, recast the expression using
boolean algebra to reduce nesting and break it up into a series
of `if' statements that each have a clear purpose.

>         if ( [[insufferable boolean expression elided]] )
>           {

Re: coding style, continuing education

[Zack Weinberg]

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

coding style, continuing education

[Mike Stump]

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 think we should all just agree that conditionals that run on
past 20 or so lines should be split.  Maybe we need a new gcc warning
when the opening ( and the closing ) are more than 20 lines apart, we
warn.  We could also have a { } checker that will warn when they are
more than, say 2000 lines apart.  Someone want to work on a useful
warning that will hit often?  :-)

If people _know_ of a good way to split this down, maybe we all just
need a refresher course on the right way, care to elaborate?


	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)))
	  {