Re: df.c and partial writes/REG_EQUAL notes

[Daniel Berlin <dan at cgsoftware dot com>]

kenner@vlsi1.ultra.nyu.edu (Richard Kenner) writes:

>     Saying "I don't think we need 3 bitmap implemenations" is not a
>     review of a patch.
>
> If the patch in question adds a third bitmap implementation, it *is* a review
> of the patch.
No, it's not.

A review of a patch tells you what is necessary to make it acceptable
for gcc. Not whether a particular person with global write access
likes it.
Often, these things are orthogonal.
Sometimes, they are not.

>
> If somebody submits a patch based on a concept with which I disagree and I
> review the patch, I'll just say that I don't think the concept behind the
> patch is reasonable. 
This is not a review of the patch itself. It is a statement about the
approach to the problem the patch takes.
That is, there are cases where review is not necessary.
Everyone agrees the approach to the actual problem the patch
implements is wrong, regardless of the implementation details or
quality of the patch.
This is not one of those cases.
If I asked someone to review a case or document for me, that i was
planning to submit to a court, and they handed it back with no marks,
comments, or anything else, and just said "I disagree", i wouldn't
feel they had reviewed it, only 
presented their opinion on the overall point of the document.
Whether they agree or not is not the point of a review. I might
disagree with the overall point as well (it all depends on who your
client is)
Nor would it change whether *others* agree with the overall point or
not.
And I would still have a document to submit.
I just wouldn't know what the heck is wrong with it.


>  In that case, there's no reason to delve further into
> the details of the patch, including the quality of the
> documentation.
If you aren't going to review it, don't review it.
That's fine.
Somebody should, of course, review each patch.  Unless *everyone*
agrees the actual approach to solving the problem is wrong.
In that case, someone should tell the contributor, too.
This is not what happened here.


>
>     Overall direction and whatnot of the compiler is not controlled by
>     [a specific global-write privilege maintainer], it's controlled by the
>     steering committee.
>
> True, but the SC has made it clear they do not want to get involved in the
> review process of individual patches, only in setting that process, which is
> that *somebody* with global write privileges must be convinced to approve the
> patch.  In the case of a *major* issue, I agree it's appropriate to raise it
> with the SC, but I doubt the SC wants to get involved at this level of
> technical detail.
Sure.
However, how can I convince *somebody* with global write access that
it's a good patch to approve, if it hasn't really been reviewed. You
can't very well do the review yourself.
It all goes back to whether a statement like "we shouldn't have 3
bitmap implementations" is a review of the patch.
Note that a review of the patch would have noticed the documentation
was deficient.
This is what would make the patch acceptable, for the most part, (at
least in this case), to other global write people.
A single global write person's opinion about a patch is not a review
of it. It is a statement that gives you some indication as to whether
or not they will review that patch. It is not a review in and of
itself.
If nobody thinks a patch is acceptable, regardless of the actual code
in the patch, then the patch is not acceptable, but it also has not
been reviewed.

>
> As to my opinion of the underlying issue, I'll say first that I have not
> looked at the patch.  However, I agree with RTH that we don't need *three*
> bitmap implementations.
Not that i disagree, i'm just curious why?
What is it about bitmaps that makes you think two is enough?
The right representation for a given set of problems is more important
than the number of representations.
In fact, it's often vital.

-- 
"If you're not part of the solution, you're part of the
precipitate.
"-Steven Wright