On Mon, 12 May 2025 at 23:15, Alejandro Colomar <a...@kernel.org> wrote: > > Hi Jonathan, > > On Mon, May 12, 2025 at 06:11:18PM +0100, Jonathan Wakely wrote: > > On 12/05/25 17:53 +0200, Alejandro Colomar wrote: > > > Suggested-by: Xavier Del Campo Romero <xavi....@tutanota.com> > > > Co-authored-by: Martin Uecker <uec...@tugraz.at> > > > Acked-by: "James K. Lowden" <jklow...@schemamania.org> > > > > What does this Acked-by: indicate? > > <https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by> > > Acked-by: may also be used by other stakeholders, such as people > with domain knowledge (e.g. the original author of the code > being modified), userspace-side reviewers for a kernel uAPI > patch or key users of a feature. > > [...] > > Acked-by: is also less formal than Reviewed-by:. For instance, > maintainers may use it to signify that they are OK with a patch > landing, but they may not have reviewed it as thoroughly as if a > Reviewed-by: was provided. Similarly, a key user may not have > carried out a technical review of the patch, yet they may be > satisfied with the general approach, the feature or the > user-facing interface. > > > My guess would be that it indicates approval for the patch, but Jim is > > not an approver for the C front end, so he can't approve this patch. > > That would be a Reviewed-by:.
In GCC I've been using Reviewed-by: for anybody who reviews a patch, not necessarily approval from a maintainer. There are only seven occurrences of Acked-by on the gcc master branch. Four of them are duplicating a Reviewed-by: trailer in the same commit which seems unnecessary. > Acked-by: can be used by a reviewer when > they like the patch but haven't reviewed as seriously as a Reviewed-by: > tag would imply. It can also be used --like in this case-- for when > someone who can't approve it, still wants to express approval. > > > Does Acked-by: indicate something other than approval? > > There are degrees of approval. The formal one would be Reviewed-by:. > The informal one would be Acked-by:. Should we agree on > > When it's > > somebody who can't approve the patch, how is it different to > > Reviewed-by:? > > Someone who can't aapprove the patch wouldn't usually emit a > Reviewed-by:. Unless they feel so strongly qualified as an exception to > review the patch (e.g., if you review a patch for the man pages about > _Atomic, you could say you've Reviewed-by, because even when you don't > have commit rights, I'm going to trust your review more than my own). > > > I'm not overjoyed by the idea of trailers that mean something in some > > other project (e.g. the kernel) but are just co-opted to mean > > something slightly (or completely) different in the GCC repo without > > some kind of agreement from the community about what they mean *here*. > > I use them with the exact meaning of > <https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by>. Yes, I read that, and "maintainer" seems to have a different meaning to how we use it in GCC. "Acked-by: is meant to be used by those responsible for or involved with the affected code in one way or another. Most commonly, the maintainer when that maintainer neither contributed to nor forwarded the patch." That sounds like approval from a maintainer (in GCC we don't "forward" patches because we only have one tree, there are no subsystem trees where work is collected then forwarded to Linus). And the description of Reviewed-by: doesn't imply approval from a maintainer, it implies a thorough review by somebody knowledgeable about the area: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#reviewer-s-statement-of-oversight I think the kernel's uses of Reviewed-by: and Acked-by: don't really map to GCC's development/review/approval model. For GCC, I think it would make more sense to use Reviewed-by: to mean somebody competent reviewed the patch, and (if we feel it's needed) something like Approved-by: to mean formal approval by a maintainer who is able to approve patches in that area. If we do want to use Acked-by: for review (possibly informal, or not a thorough review) and Reviewed-by: for formal approval by a maintainer, I'd be OK with that but I'd like to see it documented for GCC. The kernel docs don't really answer my questions about what it means for GCC, and it seems you and I are already using the trailers differently. > I would encourage using them. They convey useful information. > > > Have a lovely night! > Alex > > > > > > Signed-off-by: Alejandro Colomar <a...@kernel.org> > > > --- > > > gcc/c-family/c-common.cc | 26 +++++ > > > gcc/c-family/c-common.def | 3 + > > > gcc/c-family/c-common.h | 2 + > > > gcc/c/c-decl.cc | 22 +++- > > > gcc/c/c-parser.cc | 59 +++++++--- > > > gcc/c/c-tree.h | 4 + > > > gcc/c/c-typeck.cc | 115 +++++++++++++++++- > > > gcc/doc/extend.texi | 30 +++++ > > > gcc/testsuite/gcc.dg/countof-compile.c | 130 +++++++++++++++++++++ > > > gcc/testsuite/gcc.dg/countof-vla.c | 51 ++++++++ > > > gcc/testsuite/gcc.dg/countof.c | 154 +++++++++++++++++++++++++ > > > 11 files changed, 572 insertions(+), 24 deletions(-) > > > create mode 100644 gcc/testsuite/gcc.dg/countof-compile.c > > > create mode 100644 gcc/testsuite/gcc.dg/countof-vla.c > > > create mode 100644 gcc/testsuite/gcc.dg/countof.c > > > > > > > -- > <https://www.alejandro-colomar.es/>
