Message ID | CAMe9rOrrDCqMgT+AaLCtWgTADBc6d5ZvkCo9+5HWgGCP-qT6MQ@mail.gmail.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 112400 invoked by alias); 30 Oct 2017 20:42:14 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 112214 invoked by uid 89); 30 Oct 2017 20:42:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.9 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-oi0-f68.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=OP41MmBkTZ3eRm9lVz+XJAfSYG0+J86NHMyayVavwDw=; b=o8Iq4S4R0VYGpxOtra6oVHG6p1drnZJpjDFAoXg6xI5RbiFktPIGP/CJH7s8o72HtK ga8mK7wc4C8QGbnsnSxKlZGXy3BK4JZ0JnD3gR+kKbgJuOil/Ktx5OiJlYPy+P5h195X RxNvyXgP9rQWQR/U+kF+KqKAVnK5KjpLt6zdtXjRtynbokZ9LzEX6XO1sEq3XS2NK8GC EX19fYIpBNHICUH2wuvDl5mMgutIZYLjKMJ1cVKhtU5h+SBZx9pLwszAyQ+4I+vjOchO FLz8wPz5Czm0E3cfPMJPgqdo1WtOgRYb/u7vwvbWCTt+hHbL6GuAxRAeUgfYPHJNrxbD sbaQ== X-Gm-Message-State: AMCzsaXntPlJoqbgqKrEIZOR4vPHmgUdkseMs0Mf+UmPUV17SuBvkM+3 vpLMsw5kQVVNEVmRf+3izEIWx2QFyL1OmO2s30s= X-Google-Smtp-Source: ABhQp+StR1hJvf9gq81mzdiR67PGjtQ6KAbrk75CSE0MhqFf2yutrZEB2l6ngEnWM1TXDXynjSHLm6ksY5lAWzb4YvA= X-Received: by 10.157.43.104 with SMTP id f37mr5753542otd.491.1509396130983; Mon, 30 Oct 2017 13:42:10 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <4bcfa16d-bce2-55ed-77f2-69502b2ecfff@redhat.com> References: <20171030200246.GA14084@intel.com> <4bcfa16d-bce2-55ed-77f2-69502b2ecfff@redhat.com> From: "H.J. Lu" <hjl.tools@gmail.com> Date: Mon, 30 Oct 2017 13:42:10 -0700 Message-ID: <CAMe9rOrrDCqMgT+AaLCtWgTADBc6d5ZvkCo9+5HWgGCP-qT6MQ@mail.gmail.com> Subject: Re: [PATCH] Reformat sysdeps/x86/libc-start.c To: Florian Weimer <fweimer@redhat.com> Cc: GNU C Library <libc-alpha@sourceware.org> Content-Type: multipart/mixed; boundary="001a113d0cae36a310055cc9ac96" |
Commit Message
H.J. Lu
Oct. 30, 2017, 8:42 p.m. UTC
On Mon, Oct 30, 2017 at 1:35 PM, Florian Weimer <fweimer@redhat.com> wrote: > On 10/30/2017 09:02 PM, H.J. Lu wrote: >> >> index e11b490f5c..727d328bc7 100644 >> --- a/sysdeps/x86/libc-start.c >> +++ b/sysdeps/x86/libc-start.c >> @@ -16,13 +16,13 @@ >> <http://www.gnu.org/licenses/>. */ >> #ifndef SHARED >> -#include <ldsodefs.h> >> +# include <ldsodefs.h> >> # include <cpu-features.h> >> # include <cpu-features.c> >> extern struct cpu_features _dl_x86_cpu_features; >> -#define ARCH_INIT_CPU_FEATURES() init_cpu_features >> (&_dl_x86_cpu_features) >> +# define ARCH_INIT_CPU_FEATURES() init_cpu_features >> (&_dl_x86_cpu_features) >> #endif >> -# include <csu/libc-start.c> >> +#include <csu/libc-start.c> > > > Maybe add a /* SHARED */ comment on the #endif line while you are at it? > Sure. Why not. I checked in this to add /* !SHARED */.
Comments
On 10/30/2017 01:42 PM, H.J. Lu wrote: > On Mon, Oct 30, 2017 at 1:35 PM, Florian Weimer <fweimer@redhat.com> wrote: >> On 10/30/2017 09:02 PM, H.J. Lu wrote: >>> >>> index e11b490f5c..727d328bc7 100644 >>> --- a/sysdeps/x86/libc-start.c >>> +++ b/sysdeps/x86/libc-start.c >>> @@ -16,13 +16,13 @@ >>> <http://www.gnu.org/licenses/>. */ >>> #ifndef SHARED >>> -#include <ldsodefs.h> >>> +# include <ldsodefs.h> >>> # include <cpu-features.h> >>> # include <cpu-features.c> >>> extern struct cpu_features _dl_x86_cpu_features; >>> -#define ARCH_INIT_CPU_FEATURES() init_cpu_features >>> (&_dl_x86_cpu_features) >>> +# define ARCH_INIT_CPU_FEATURES() init_cpu_features >>> (&_dl_x86_cpu_features) >>> #endif >>> -# include <csu/libc-start.c> >>> +#include <csu/libc-start.c> >> >> >> Maybe add a /* SHARED */ comment on the #endif line while you are at it? >> > > Sure. Why not. I checked in this to add /* !SHARED */. Should we extend consensus? https://sourceware.org/glibc/wiki/Consensus#Trivial_Bug-Fix_Changes ~~~ Anyone can commit a change fixing obvious coding standards problems in a recently committed patch. Post the patch and ChangeLog to libc-alpha with a short message and then push the commit. ~~~ s/a recently/any/g We normally allow this kind of change for "recently committed" patches, but shy away from it for older changes because of the impact it might ave on established code. In this case I would have liked HJ to be able to just push the cleanup without anyone *needing* to do a pre-commit review.
Hi, Carlos O'Donell wrote: > Should we extend consensus? > > https://sourceware.org/glibc/wiki/Consensus#Trivial_Bug-Fix_Changes > ~~~ > Anyone can commit a change fixing obvious coding standards problems > in a recently committed patch. Post the patch and ChangeLog to > libc-alpha with a short message and then push the commit. > ~~~ > s/a recently/any/g > > We normally allow this kind of change for "recently committed" > patches, but shy away from it for older changes because of the impact > it might ave on established code. > > In this case I would have liked HJ to be able to just push the cleanup > without anyone *needing* to do a pre-commit review. I would rather not --- getting LGTM is a pretty lightweight action, and in other projects when I thought I had a good cleanup, getting review pushed me in another direction that caused an even better result. I'd rather not see glibc switching to post-push review for most commits and pre-push review as an exception. Instead, how can I help with making pre-push review work better? E.g. if obvious patches are stalling without review, can we figure out why and get that problem solved? (E.g. do we need something like patchwork to keep track of patches needing review?) Thanks, Jonathan
On 10/30/2017 11:12 PM, Jonathan Nieder wrote: > Hi, > > Carlos O'Donell wrote: > >> Should we extend consensus? >> >> https://sourceware.org/glibc/wiki/Consensus#Trivial_Bug-Fix_Changes >> ~~~ >> Anyone can commit a change fixing obvious coding standards problems >> in a recently committed patch. Post the patch and ChangeLog to >> libc-alpha with a short message and then push the commit. >> ~~~ >> s/a recently/any/g >> >> We normally allow this kind of change for "recently committed" >> patches, but shy away from it for older changes because of the impact >> it might ave on established code. >> >> In this case I would have liked HJ to be able to just push the cleanup >> without anyone *needing* to do a pre-commit review. > > I would rather not --- getting LGTM is a pretty lightweight action, I must say that I agree with Carlos here. Even what should be a trivial review is often difficult to get, and a lot of cleanups land only because the author eventually commits their changes without review. Thanks, Florian
Florian Weimer wrote: > On 10/30/2017 11:12 PM, Jonathan Nieder wrote: >> Carlos O'Donell wrote: >>> Should we extend consensus? >>> >>> https://sourceware.org/glibc/wiki/Consensus#Trivial_Bug-Fix_Changes >>> ~~~ >>> Anyone can commit a change fixing obvious coding standards problems >>> in a recently committed patch. Post the patch and ChangeLog to >>> libc-alpha with a short message and then push the commit. >>> ~~~ >>> s/a recently/any/g [...] >> I would rather not --- getting LGTM is a pretty lightweight action, > > I must say that I agree with Carlos here. Even what should be a trivial > review is often difficult to get, and a lot of cleanups land only because > the author eventually commits their changes without review. Sounds like a plan. I don't want to lose sight of this being a symptom of the review process being broken, though. My offer to help in whatever way looks most useful still stands: e.g. feel free to explicitly cc me if you have an obvious change that doesn't fall into this 'coding standards' category that needs review. Thanks, Jonathan
On Fri, Nov 3, 2017 at 1:51 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: > Florian Weimer wrote: >> On 10/30/2017 11:12 PM, Jonathan Nieder wrote: >>> Carlos O'Donell wrote: > >>>> Should we extend consensus? >>>> >>>> https://sourceware.org/glibc/wiki/Consensus#Trivial_Bug-Fix_Changes >>>> ~~~ >>>> Anyone can commit a change fixing obvious coding standards problems >>>> in a recently committed patch. Post the patch and ChangeLog to >>>> libc-alpha with a short message and then push the commit. >>>> ~~~ >>>> s/a recently/any/g > [...] >>> I would rather not --- getting LGTM is a pretty lightweight action, >> >> I must say that I agree with Carlos here. Even what should be a trivial >> review is often difficult to get, and a lot of cleanups land only because >> the author eventually commits their changes without review. > > Sounds like a plan. > > I don't want to lose sight of this being a symptom of the review > process being broken, though. My offer to help in whatever way looks > most useful still stands: e.g. feel free to explicitly cc me if you > have an obvious change that doesn't fall into this 'coding standards' > category that needs review. Here is one: https://sourceware.org/ml/libc-alpha/2017-10/msg01322.html
On 11/03/2017 02:01 PM, H.J. Lu wrote: > On Fri, Nov 3, 2017 at 1:51 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: >> Florian Weimer wrote: >>> On 10/30/2017 11:12 PM, Jonathan Nieder wrote: >>>> Carlos O'Donell wrote: >> >>>>> Should we extend consensus? >>>>> >>>>> https://sourceware.org/glibc/wiki/Consensus#Trivial_Bug-Fix_Changes >>>>> ~~~ >>>>> Anyone can commit a change fixing obvious coding standards problems >>>>> in a recently committed patch. Post the patch and ChangeLog to >>>>> libc-alpha with a short message and then push the commit. >>>>> ~~~ >>>>> s/a recently/any/g >> [...] >>>> I would rather not --- getting LGTM is a pretty lightweight action, >>> >>> I must say that I agree with Carlos here. Even what should be a trivial >>> review is often difficult to get, and a lot of cleanups land only because >>> the author eventually commits their changes without review. >> >> Sounds like a plan. >> >> I don't want to lose sight of this being a symptom of the review >> process being broken, though. My offer to help in whatever way looks >> most useful still stands: e.g. feel free to explicitly cc me if you >> have an obvious change that doesn't fall into this 'coding standards' >> category that needs review. > > Here is one: > > https://sourceware.org/ml/libc-alpha/2017-10/msg01322.html Likewise we have a list which needs review: https://patchwork.sourceware.org/project/glibc/list/
On 10/30/2017 03:12 PM, Jonathan Nieder wrote: > Hi, > > Carlos O'Donell wrote: > >> Should we extend consensus? >> >> https://sourceware.org/glibc/wiki/Consensus#Trivial_Bug-Fix_Changes >> ~~~ >> Anyone can commit a change fixing obvious coding standards problems >> in a recently committed patch. Post the patch and ChangeLog to >> libc-alpha with a short message and then push the commit. >> ~~~ >> s/a recently/any/g >> >> We normally allow this kind of change for "recently committed" >> patches, but shy away from it for older changes because of the impact >> it might ave on established code. >> >> In this case I would have liked HJ to be able to just push the cleanup >> without anyone *needing* to do a pre-commit review. > > I would rather not --- getting LGTM is a pretty lightweight action, > and in other projects when I thought I had a good cleanup, getting > review pushed me in another direction that caused an even better > result. The point I made is not about any general changes, but about a specific subset of changes, namely coding standard cleanups and trivial changes. While perhaps there is a *better* result, the immediate incremental benefit of HJ having checked in his patch, and moved on to another fix, is worth codifying as acceptable community practice without needing immediate review. > I'd rather not see glibc switching to post-push review for most > commits and pre-push review as an exception. Instead, how can I help > with making pre-push review work better? We have very narrow criteria for post-push review. I do not think that this will get abused by any serious developer in this community, and if we see such abuse we can stop it. We have post-push review for specific things mentioned here: https://sourceware.org/glibc/wiki/Consensus Even if we had more reviewer resources, having consensus only helps accelerate the review for everyone (and you have to have commit access in the first place). > E.g. if obvious patches are stalling without review, can we figure out > why and get that problem solved? (E.g. do we need something like > patchwork to keep track of patches needing review?) How long is too long? 1h, 2h, 4h, 1day? What if you're working on the weekend cleaning stuff up and nobody else is around to ack your cleanup? We can have established consensus on some things, and having it just helps make things move smoothly.
On 10/30/2017 03:12 PM, Jonathan Nieder wrote:
> Instead, how can I help with making pre-push review work better?
I want to give you a large thank you for publicly offering to help.
Every person who submits patches, or files bugs, also gets my thanks.
Please don't let the ensuing discussion dissuade you from helping :-)
We have a patch tracker, and several of us use it to track specific
things we are doing, having other people tracking it for trivial patches
to review and ACK would be awesome.
On Fri, 3 Nov 2017, Carlos O'Donell wrote: > The point I made is not about any general changes, but about a specific > subset of changes, namely coding standard cleanups and trivial changes. The difficulty is that there are many subtleties regarding the coding standards and it would be easy for someone to produce a very large patch making changes that superficially follow the standards but are not in fact desirable. * E.g. if someone globally added spaces before '(' before we'd explicitly documented the rule about macros that logically expand to a single symbol name. * E.g. if someone applied the rule for preprocessor indentation without following the rule that the outer multiple-include guard does not count for the purposes of indentation for nested directives. * E.g. if someone reformatted a file shared with another project without considering if the variations from normal glibc practice are to facilitate sharing with that project. The effect of that is that even if we think some such changes are obvious and appropriate for commit without review, they should cease to become obvious if the patch, or the total set of such patches from one contributor in some period, gets too big, to ensure there is time for issues to be raised before the same mistake has been made too many times. > How long is too long? 1h, 2h, 4h, 1day? What if you're working on the > weekend cleaning stuff up and nobody else is around to ack your cleanup? What I suggest above would imply we do *not* want someone committing a large set of cleanups over the weekend while no-one is looking at that, precisely because there could be a global issue with one person's understanding of the standards that should be pointed out before many such changes have gone in.
On 11/03/2017 11:05 PM, Joseph Myers wrote: > What I suggest above would imply we do*not* want someone committing a > large set of cleanups over the weekend while no-one is looking at that, > precisely because there could be a global issue with one person's > understanding of the standards that should be pointed out before many such > changes have gone in. We should also avoid invasive cleanups of files on which others are working. This requires coordination via the mailing list anyway. Thanks, Florian
From 4ad5106e3b04cc7630f7dbfdb25369807f532843 Mon Sep 17 00:00:00 2001 From: "H.J. Lu" <hjl.tools@gmail.com> Date: Mon, 30 Oct 2017 13:39:31 -0700 Subject: [PATCH] sysdeps/x86/libc-start.c: Add /* !SHARED */ * sysdeps/x86/libc-start.c: Add /* !SHARED */. --- ChangeLog | 4 ++++ sysdeps/x86/libc-start.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 4751a83927..c99898e127 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,9 @@ 2017-10-30 H.J. Lu <hongjiu.lu@intel.com> + * sysdeps/x86/libc-start.c: Add /* !SHARED */. + +2017-10-30 H.J. Lu <hongjiu.lu@intel.com> + * sysdeps/x86/libc-start.c: Reformat. 2017-10-30 H.J. Lu <hongjiu.lu@intel.com> diff --git a/sysdeps/x86/libc-start.c b/sysdeps/x86/libc-start.c index 727d328bc7..7a1f51c52e 100644 --- a/sysdeps/x86/libc-start.c +++ b/sysdeps/x86/libc-start.c @@ -24,5 +24,5 @@ extern struct cpu_features _dl_x86_cpu_features; # define ARCH_INIT_CPU_FEATURES() init_cpu_features (&_dl_x86_cpu_features) -#endif +#endif /* !SHARED */ #include <csu/libc-start.c> -- 2.13.6