Re: [Fwd: Commit approval pls: PR 9877, 8336, plus other stuff]

[Bruce Korb <bkorb at veritas dot com>]

Kean Johnston wrote:
> 
> Bruce Korb wrote:
> > to the cosmetic change to htonl.
> This is absolutely required. GCC 3.3/3.4 no longer accept multi-line

Oops.  I confess I was assuming that this would have been hit before,
as I have no platform that it triggers on.  Sorry.  Never mind.
I don't use asm myself, so I didn't know.

> Ok I guess I can refine it somehwat, but this was lifted from the 3.4
> tip which you had already approved a few months back.

> > sco_regset:  please remove several of the ``\n''-s.  If a ripple shows up in
> > the result, I'd like the ``diff -c'' to cover the test name, too.  (i.e., the
> > ``#ifndef SCO_REGSET_CHECK'' and ``#endif /* SCO_REGSET_CHECK */'' are really
> > visual tags that ought to show up in a ``diff -c''.)
> Sorry I may be dense but I really dont know what you are asking for
> here. The test text is representative of what is being fixed. It applies
> to one platform only, so I am not sure about what "ripples" you are
> refering to.

The purpose of the test text is:

1.  to ensure the header change you are trying to make works the
    way you expect with the sources you are having problems with
2.  to ensure that you do not break other fixes
3.  to ensure that future changes do not break your fix

If someone's future fix breaks yours, it is easier to identify the
fix that they have broken if either ``#ifndef SCO_REGSET_CHECK'' or
``#endif /* SCO_REGSET_CHECK */'' show up in the ``diff -c'' output.
I ought to explain why it is useful to keeps tests short in the
readme.

> >> * fixinc/tests/base/math.h: Update
> > You'll notice that the fix was not applied.
> > Please examine the results of the "make check" and check that
> > what you intended to have happen has, in fact, happened.
> I did, and it did. I am not sure what you mean when you say the fix was
> not applied. make check reports success, and the fixed math.h looks
> quite fine.

As it appears:

> Index: gcc/fixinc/tests/base/math.h
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/fixinc/tests/base/math.h,v
> retrieving revision 1.9.20.1
> diff -u -r1.9.20.1 math.h
> --- gcc/fixinc/tests/base/math.h        1 Mar 2003 20:41:38 -0000       1.9.20.1
> +++ gcc/fixinc/tests/base/math.h        5 Aug 2003 19:08:04 -0000
> @@ -104,6 +104,13 @@
>  #endif  /* RS6000_DOUBLE_CHECK */
> 
> 
> +#if defined( SCO_MATH_CHECK )
> +#define __fp_class(a) \
> + __builtin_generic(a,"ld:__fplcassifyl;f:__fpclassifyf;:__fpclassify")
> +
> +#endif  /* SCO_MATH_CHECK */
> +
> +
>  #if defined( STRICT_ANSI_NOT_CTD_CHECK )
>  #if 1 && \
>  && defined(mumbling) |& ( !defined(__STRICT_ANSI__)) \

As it *SHOULD* appear _after_ the fix:

> Index: gcc/fixinc/tests/base/math.h
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/fixinc/tests/base/math.h,v
> retrieving revision 1.9.20.1
> diff -u -r1.9.20.1 math.h
> --- gcc/fixinc/tests/base/math.h        1 Mar 2003 20:41:38 -0000       1.9.20.1
> +++ gcc/fixinc/tests/base/math.h        5 Aug 2003 19:08:04 -0000
> @@ -104,6 +104,13 @@
>  #endif  /* RS6000_DOUBLE_CHECK */
> 
> 
> +#if defined( SCO_MATH_CHECK )
> +#ifndef __GNUC__
> +#define __fp_class(a) \
> + __builtin_generic(a,"ld:__fplcassifyl;f:__fpclassifyf;:__fpclassify")
> "#else
> +#define __fp_class(a) \
> + __builtin_choose_expr(__builtin_types_compatible_p(typeof(a),long double),\
> + __fpclassifyl(a), \
> + __builtin_choose_expr(__builtin_types_compatible_p(typeof(a), float), \
> + __fpclassifyf(a),__fpclassify(a)))
> +#endif
> +
> +#endif  /* SCO_MATH_CHECK */
> +
> +
>  #if defined( STRICT_ANSI_NOT_CTD_CHECK )
>  #if 1 && \
>  && defined(mumbling) |& ( !defined(__STRICT_ANSI__)) \

It does not look this way because the test text did not contain
"inline double abs", your selection criterion.  The fix was not applied.

> >> * fixinc/tests/base/sys/byteorder.h: Update
> >
> > Unnecessary.
> Why? The text changed. htonl() was updated.

OK.  It's necessary.  Sorry.

> Ok thanks for pointing out how to fix this. I just assumed it was a
> weakness in the autogen expansions of all of the tests that it was
> taking precedence of one over the other. I followed your instructions in
> the readme to the letter, and this was the result.

What you did was pretty good.  I just didn't explain all the possible interactions.
I guess I should add a section on what to do if make check doesn't go as
expected. ...  *Two* readme fixes, I guess.

> I'll make the other improvements now though.

Thanks - Bruce