Re: build_range_type/build_index_type

[Mark Mitchell <mark at markmitchell dot com>]

>>>>> "Jeffrey" == Jeffrey A Law <law@hurl.cygnus.com> writes:

    >> I'll submit a patch that doesn't change the behavior of
    >> build_index_type or build_range_type,
    Jeffrey> Fine.

    >> but does make build_index_2_type behave like build_index_type
    >> rather than build_range_type, and which also breaks out the
    >> common code.  OK?

    Jeffrey> I'd like to see more information about why you think that
    Jeffrey> is the right change to make -- not from a "clean things
    Jeffrey> up" standpoint, but from an implementation correctness
    Jeffrey> standpoint.

Of course.

    Jeffrey> We've got a lot of legacy code which needs to be better
    Jeffrey> understood before we start changing it.  Changing it,
    Jeffrey> then hoping for the best is the wrong approach.

Well, in this case, some hapless soul had written, in the C++
front-end:

  build_index_2_type (zero_size_node, n);

instead of just:

  build_index_type (n);

Clearly, these should do the same thing.  However, the implementation
of build_index_2_type calls build_range_type (which, as you point out
is trying to solve a different problem!).  In this case n was an
INTEGER_CST for -1, indicating an array of unknown bound.  But,
build_index_2_type (via build_range_type) munged this into an array of
bound 2^32 - 1, which is entirely different.  This caused problems
later on.  

I fixed the problem by using build_index_type in the C++ front-end
instead of the old build_index_2_type.  But, to use Craig's
terminology, the equivalence of these statements is "clearly correct".
The fact that changing this affected behavior indicates a problem
somewhere.

In other words, if changing build_index_2_type to work like
build_index_type breaks anything then we do want to know that.  That
indicates a real problem *somehwere else* which we are just papering
over.  Even if making this change broke lots of stuff, I would
continue to think that the change was correct; this is Craig's
definition of "clearly correct".

In practice, though, making this change (i.e., making
build_index_2_type act like build_index_type) has not affected
anything in the various test suites, or in the real code I've been
working with.

I understand your caution, but I think maybe you're just nervous here
because of the problems arising from Kenner's change.  Imagine this
were the back-end, and you saw that emit_insns, when fed just one
insn, did something very different than add_insn.  That just wouldn't
make much sense.  I bet you'd poke around a little, make the change,
see if anything broke, and if not, check it in.

-- 
Mark Mitchell 			mark@markmitchell.com
Mark Mitchell Consulting	http://www.markmitchell.com