Re: Patch: add prefix to condition args in opcodes

[Jeffrey A Law <law@cygnus.com>]

  In message <199907092055.NAA11875@cygnus.com>you write:
  > OK, here's a patch to prefix all the condition arg characters with '?'.  It
  > may look a bit large, but all I've done is regroup all the cases for
  > conditions inside the '?' case and do a bit of renaming.  It passes the
  > test suite and appears to work fine.
  > 
  > Because all the conditions are in a prefix, I just added extra codes to
  > deal with various 64 bit conditions.  You'll see that there are some stubs
  > for different new conditions that aren't filled in yet.
This should have been a completely separate patch.  There is *no* reason
to include this stuff as part of the basic reorganization.  And by doing so
all you end up doing is making more work for me as I try to sort out if any
of the PA2.0 stuff is actually correct.

*PLEASE* submit independent changes as independent patches.  Anything else
just makes more work for me because large patches are simply harder to
properly review and problems in one area can easily cause the whole patch
to be held up.  It is better for everyone if you submit independent changes
as separate patches.


  > Let me know what you think.  If this is OK, I suggest we prefix the float
  > args next.
I'm happy with the general concept, but the code itself needs some work.

First, you reversed the patch.  ie, you did diff new old.  Not a major problem,
but it did take a few seconds to realize what had happened (I kept looking for
something nested inside the '?' case in the "new" code and couldn't find it).


Anyway the code itself.

                    /* Handle an add condition.  */
                  case 'A':     /* 64 bit add */
                    if (*s == ',' && *(s + 1) == '*')
                      {
                        cond_64 = 1;
                        s += 2; /* Eat up the ,* */
                      }
                    else break;

First, we do not generally put comments after code; comments belong on separate
lines before the code to which they apply.

Second, we do not write "else break;".  We write

   else
     break;

You made that goof in a variety of places.

Similarly in other places you do stuff like the following:

    if (!cond_64) s++;

We generally try to avoid that kind of style.

  
I'm not particular happy with the way 'A' falls into 'a'.  It may be cleaner
to do

	case 'A':
	case 'a':
	  if (*args == 'A')
	    {
	       Do the few "A" specific things
	    }
	  Do generic code for 'A' and 'a'.

The flow control (at least to me) is easier to follow with that kind of style
(it's still not ideal, but I'm not sure what else we can do to clean this up
without starting over with a complete redesign).

This happens in other places too.

These lines in the 'a' handling are indented improperly:

                  opcode |= cmpltr << 13;
                  INSERT_FIELD_AND_CONTINUE (opcode, flag, 12);

They need to be moved two columns to the right.  Similar problems occur with
the '@' handling.

In the 'b' case the INSERT_FIELD_AND_CONTINUE needs to be moved to the left
two columns.

Do not write "abort()", instead write "abort ()".  Seems like a nit, but
we need to try and be consistent with the coding standards.

You have some comments that wrap when viewed on an 80 column screen.  They need
to be cleaned up.

This code in 'b' does not work if we had previously come in from the 'B' case
and had found a 64bit completer completer.  *s will have already been
advanced past the ,*:


                    cmpltr = 0;
                    if (*s == ',')
                      {
                        s++;



I believe you made a similar mistake in the 'X' case where it falls into
'x'.

I'm not sure the disassembler changes are right, particularly in how they
handle the 64bit stuff.  But then again,  I would claim we shouldn't be
trying to add the 64bit stuff as part of this patch anyway.

Please break out the basic reorganization from the 64bit stuff.  Let's get
the reorgs done and installed, then we can add the 64bit stuff separately.


jeff