| Message ID | 20171030200246.GA14084@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Reformat sysdeps/x86/libc-start.c | expand |
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? Thanks, Florian
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 */.
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
diff --git a/ChangeLog b/ChangeLog index 5ea3d856a1..4751a83927 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,9 @@ 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> + [BZ #22353] * sysdeps/i386/i586/strcpy.S (STRCPY): Use conditional branches. (1): Renamed to ... diff --git a/sysdeps/x86/libc-start.c b/sysdeps/x86/libc-start.c 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>