Re: [PATCH] accept all C integer types in function parameters referenced by alloc_align (PR 88363)

[Martin Sebor <msebor at gmail dot com>]

On 12/11/18 11:15 AM, Marek Polacek wrote:
> On Tue, Dec 11, 2018 at 09:59:27AM -0700, Martin Sebor wrote:
> > [*] The change in the patch is obvious enough to me.  All it
> > does is accept more of the things that are accepted by GCC 8
> > (enums, bools, wchar_t, etc.) and that inadvertently started
> > to be rejected as a result of my prior change.   That the rules
> > can be made more restrictive is something different.
>   
> Obvious changes should be obvious to anyone, not just the committer, IMHO.  I
> don't think we should make the rules more restrictive; 

I was referring to the rules of what expression the compiler accepts
as positional parameters.

> what we have in place
> seems to have worked fine and I would have thought it's clear that changing
> what the compiler accepts will never be an obvious change, unlike, say, fixing
> a test that fails with -m32 because it uses 'unsigned long' instead of size_t.

If that's the intent it should be stated in the policy then.  Seems
simple enough:

  Obvious fixes <ins>that have no impact on constructs accepted
  or rejected by the compiler</ins> can be committed without
  prior approval.

I have no problem with this clarification.  I just wouldn't have
thought it to apply to restoring previous behavior that I myself
had inadvertently removed (i.e., fixing my own regression).

> > I recently brought up the question of the write w/o approval
> > policy on the gcc list:
> >
> >    https://gcc.gnu.org/ml/gcc/2018-11/msg00165.html
> >
> > looking for clarification.  Except for Jeff's comment (which
> > I'm afraid didn't really clarify things), didn't get any.
> >
> > You (the maintainers) have put it in place.  If you don't intend
> > for the rest of us to make use of it, or if it's not meant to be
> > interpreted to give us the freedom to decide what is or isn't
> > obvious, then change it. But it's disingenuous to claim that "We
> > don't want to get overly restrictive about checkin policies" and
> > then chastise people each time they say they might check something
> > in on their own.
>
> ...or fixing typos in comments and formatting fixes should be obvious, adding
> new tests for fixed bugs likely as well, but outlining semantics in a comment
> doesn't strike me as obvious at all.  "When in doubt, ask for a review."

I was not in doubt, but I posted the patch for review just
the same, didn't I?

If by "outlining semantics in a comment" you are referring to
the patch mentioned in the other thread then if such changes are
also not meant to fall under obvious then again, update the policy:

  Obvious fixes <ins>that have no impact on constructs accepted
  or rejected by the compiler and that do not outline the semantics
  of GCC internals in comments</ins> can be committed without prior
  approval.

In both of these cases I posted the patch and invited feedback
to make sure that was seemed clearly correct to me wouldn't be
viewed differently by others.  That still seems perfectly
appropriate to me.  But if even that isn't acceptable then update
the policy to make it clear that posting a patch and saying that
you'll commit it if no one objects is also against the rules.

Martin