Re: RFA: osabi: correct test for compatible handlers

[Jim Blandy <jimb at redhat dot com>]

Andrew Cagney <ac131313@redhat.com> writes:

> > Andrew Cagney <ac131313@redhat.com> writes:
> >
> >> > +   /* BFD's 'A->compatible (A, B)' functions return zero if A and B are
> >> > +      incompatible.  But if they are compatible, it returns the 'more
> >> > +      featureful' of the two arches.  That is, if A can run code
> >> > +      written for B, but B can't run code written for A, then it'll
> >> > +      return A.
> >> > + +      struct bfd_arch_info objects are atoms: that is, there's
> >> > supposed
> >> > +      to be exactly one instance for a given machine.  So you can tell
> >> > +      whether two are equivalent by comparing pointers.  */
> >> > +   return (a == b || a->compatible (a, b) == a);
> >
> >> Hey, nice.
> >> Don't worry about a can_run_code_for function though, having the
> >> logic
> >> inline makes what's happening easier to understand (and will simplify
> >> a follow-on wild-card patch I've got pending).
> > It may be easier for you, but the original author did get the test
> > backwards, and I had to go through an embarrassing number of wrong
> > tries before I got it right.  I'd really like to leave the function
> > separate.
> 
> I had to go through an equally enbarrassing number of tries before I
> established exactly what the patch was doing.  Changing:
> 
> 	if (compatible == handler->arch_info)
> 
> to:
> 
> 	if (compatible == arch_info)
> 
> (correct?)  The really important thing here is your commentary as that
> explains exactly what is going on.  Having it as close as possible to
> the problem (the call site) is, I think, going to make things easier
> to understand.

I agree it's harder to see what the *patch* does when I pull
everything out into a separate function --- you have to expand the
function in-place, and then compare before and after.

But I think it's easier to see what the *resulting code* does with the
function in place.  We should put the readability of the resultant
code above readability of the change.  You say, "A can use a handler
for B if A can run code for B", and then you can make a separate check
to see whether can_run_code_for is correct.