Re: ip2k port reviewing/accepting

[Geoff Keating <geoffk at geoffk dot org>]

Denis Chertykov <denisc@overta.ru> writes:

> Please !
> Anybody tell me how many time must I wait ip2k port reviewing ?

For some reason, your patch didn't make it into the newsgroup we have
here, thus the delay.  A week is not very long for review of a large
patch, though.


It seems generally OK, but:

- It needs a ChangeLog entry
- You need to update the documentation, in particular the constraints
  need to be documented
- In cases like this:

#if 0 /* FIXME: denisc@overta.ru */
  /* If we're doing a DImode compare then force any CONST_INT second
     operand to be CONST_DOUBLE */
  if (GET_MODE (x) == DImode
      && GET_CODE (y) == CONST_INT)
    y = gen_rtx_CONST_DOUBLE (VOIDmode, const0_rtx,
		              INTVAL (y) & 0xffffffff,
		              (INTVAL (y) & 0x80000000 ? 0xffffffff : 0));
#endif

  it's better to just delete the code.

- Don't use comments to conditionally compile code, like:
  /*
  if (can_use_skip)
    {
      fprintf(stdout, "\r\nCan use skip %d (signed): %d",
	      can_use_skip, INSN_UID (insn));
    }
  */

  it's better to use '#if 0' or similar.  There are many instances, be
  sure to get them all.  For some of them, for instance in
  mdr_try_move_dp_reload, it's probably better to just delete the code.

- Generally, comments should be full sentences, for example:

struct we_jump_targets
{
  int target; /* Is this a jump target? */
  int reach_count; /* Number of ways we can reach this insn */
  int touch_count; /* Number of times we've touched this isns during
                      scanning */
  rtx w_equiv; /* WREG-equivalence at this point */
};

  should have a '.' at the end of the sentences, followed by two spaces.
  Try "egrep '([^?!.]  |[^ ] )\*/' gcc/config/ip2k/*" to find them
  all.
  
  Also, 'insn' is mis-spelt.

There are some minor things that you might consider doing now or after
the port is committed:

- change all occurrences of "gen_rtx (XXX, " into "gen_rtx_XXX (" for
  all 'XXX', for instance in INIT_TARGET_OPTABS.  There are many
  instances, be sure to get them all.

-- 
- Geoffrey Keating <geoffk@geoffk.org> <geoffk@redhat.com>