| Message ID | 20210423203959.78275-6-imp@bsdimp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,01/24] bsd-user: whitespace changes | expand |
On 4/23/21 1:39 PM, imp@bsdimp.com wrote: > From: Warner Losh<imp@bsdimp.com> > > Lines > 80 or 90 characters > C++ comments > BSD /*- block comment convention removed. > > Signed-off-by: Warner Losh<imp@bsdimp.com> > --- > bsd-user/bsd-mman.h | 42 ++++++++++++++++++++++++++---------------- > 1 file changed, 26 insertions(+), 16 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Fri, Apr 23, 2021 at 02:39:50PM -0600, imp@bsdimp.com wrote: > From: Warner Losh <imp@bsdimp.com> > > Lines > 80 or 90 characters > C++ comments > BSD /*- block comment convention removed. > > Signed-off-by: Warner Losh <imp@bsdimp.com> > --- > bsd-user/bsd-mman.h | 42 ++++++++++++++++++++++++++---------------- > 1 file changed, 26 insertions(+), 16 deletions(-) > > diff --git a/bsd-user/bsd-mman.h b/bsd-user/bsd-mman.h > index 910e8c1921..5a64d0d425 100644 > --- a/bsd-user/bsd-mman.h > +++ b/bsd-user/bsd-mman.h > @@ -1,4 +1,4 @@ > -/*- > +/* > * Copyright (c) 1982, 1986, 1993 > * The Regents of the University of California. All rights reserved. > * > @@ -30,16 +30,20 @@ > * $FreeBSD: src/sys/sys/mman.h,v 1.42 2008/03/28 04:29:27 ps Exp $ > */ > > -#define TARGET_FREEBSD_MAP_RESERVED0080 0x0080 /* previously misimplemented MAP_INHERIT */ > -#define TARGET_FREEBSD_MAP_RESERVED0100 0x0100 /* previously unimplemented MAP_NOEXTEND */ > -#define TARGET_FREEBSD_MAP_STACK 0x0400 /* region grows down, like a stack */ > -#define TARGET_FREEBSD_MAP_NOSYNC 0x0800 /* page to but do not sync underlying file */ > +#define TARGET_FREEBSD_MAP_RESERVED0080 0x0080 > + /* previously misimplemented MAP_INHERIT */ > +#define TARGET_FREEBSD_MAP_RESERVED0100 0x0100 > + /* previously unimplemented MAP_NOEXTEND */ > +#define TARGET_FREEBSD_MAP_STACK 0x0400 > + /* region grows down, like a stack */ > +#define TARGET_FREEBSD_MAP_NOSYNC 0x0800 > + /* page to but do not sync underlying file */ I find this indented following comment style more ambiguous as to what constant the comment applies to. IMHO would be clearer as /* previously misimplemented MAP_INHERIT */ #define TARGET_FREEBSD_MAP_RESERVED0080 0x0080 /* previously unimplemented MAP_NOEXTEND */ #define TARGET_FREEBSD_MAP_RESERVED0100 0x0100 /* region grows down, like a stack */ #define TARGET_FREEBSD_MAP_STACK 0x0400 /* page to but do not sync underlying file */ #define TARGET_FREEBSD_MAP_NOSYNC 0x0800 Regards, Daniel
On Mon, 26 Apr 2021 at 10:01, Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Fri, Apr 23, 2021 at 02:39:50PM -0600, imp@bsdimp.com wrote: > > -#define TARGET_FREEBSD_MAP_RESERVED0080 0x0080 /* previously misimplemented MAP_INHERIT */ > > -#define TARGET_FREEBSD_MAP_RESERVED0100 0x0100 /* previously unimplemented MAP_NOEXTEND */ > > -#define TARGET_FREEBSD_MAP_STACK 0x0400 /* region grows down, like a stack */ > > -#define TARGET_FREEBSD_MAP_NOSYNC 0x0800 /* page to but do not sync underlying file */ > > +#define TARGET_FREEBSD_MAP_RESERVED0080 0x0080 > > + /* previously misimplemented MAP_INHERIT */ > > +#define TARGET_FREEBSD_MAP_RESERVED0100 0x0100 > > + /* previously unimplemented MAP_NOEXTEND */ > > +#define TARGET_FREEBSD_MAP_STACK 0x0400 > > + /* region grows down, like a stack */ > > +#define TARGET_FREEBSD_MAP_NOSYNC 0x0800 > > + /* page to but do not sync underlying file */ > > I find this indented following comment style more ambiguous as to > what constant the comment applies to. IMHO would be clearer as > > /* previously misimplemented MAP_INHERIT */ > #define TARGET_FREEBSD_MAP_RESERVED0080 0x0080 > > /* previously unimplemented MAP_NOEXTEND */ > #define TARGET_FREEBSD_MAP_RESERVED0100 0x0100 > > /* region grows down, like a stack */ > #define TARGET_FREEBSD_MAP_STACK 0x0400 > > /* page to but do not sync underlying file */ > #define TARGET_FREEBSD_MAP_NOSYNC 0x0800 Or alternatively decide that this is one of those cases where the coding style's "If wrapping the line at 80 columns is obviously less readable and more awkward, prefer not to wrap it" advice applies. The lines as they stand are only 95 characters or so long. thanks -- PMM
On Mon, Apr 26, 2021 at 3:13 AM Peter Maydell <peter.maydell@linaro.org> wrote: > On Mon, 26 Apr 2021 at 10:01, Daniel P. Berrangé <berrange@redhat.com> > wrote: > > > > On Fri, Apr 23, 2021 at 02:39:50PM -0600, imp@bsdimp.com wrote: > > > -#define TARGET_FREEBSD_MAP_RESERVED0080 0x0080 /* previously > misimplemented MAP_INHERIT */ > > > -#define TARGET_FREEBSD_MAP_RESERVED0100 0x0100 /* previously > unimplemented MAP_NOEXTEND */ > > > -#define TARGET_FREEBSD_MAP_STACK 0x0400 /* region grows down, > like a stack */ > > > -#define TARGET_FREEBSD_MAP_NOSYNC 0x0800 /* page to but do not > sync underlying file */ > > > +#define TARGET_FREEBSD_MAP_RESERVED0080 0x0080 > > > + /* previously misimplemented > MAP_INHERIT */ > > > +#define TARGET_FREEBSD_MAP_RESERVED0100 0x0100 > > > + /* previously unimplemented > MAP_NOEXTEND */ > > > +#define TARGET_FREEBSD_MAP_STACK 0x0400 > > > + /* region grows down, like a stack */ > > > +#define TARGET_FREEBSD_MAP_NOSYNC 0x0800 > > > + /* page to but do not sync > underlying file */ > > > > I find this indented following comment style more ambiguous as to > > what constant the comment applies to. IMHO would be clearer as > > > > /* previously misimplemented MAP_INHERIT */ > > #define TARGET_FREEBSD_MAP_RESERVED0080 0x0080 > > > > /* previously unimplemented MAP_NOEXTEND */ > > #define TARGET_FREEBSD_MAP_RESERVED0100 0x0100 > > > > /* region grows down, like a stack */ > > #define TARGET_FREEBSD_MAP_STACK 0x0400 > > > > /* page to but do not sync underlying file */ > > #define TARGET_FREEBSD_MAP_NOSYNC 0x0800 > > Or alternatively decide that this is one of those cases where the coding > style's "If wrapping the line at 80 columns is obviously less readable and > more awkward, prefer not to wrap it" advice applies. The lines as they > stand > are only 95 characters or so long. > I'm cool either way. Projects differ about how rigid or flexible style can be. I noticed this when cleaning up another thing. If we can apply that advice, I'd prefer that. Maybe it's better to just drop this entirely. In the final state, this file is here, but none of these flags are actually used. Not sure why they aren't, but there's commentary that's explicit about using the host constants in many places. Rather than take the time to sort this all out right now (it is a minor detail I think in the scheme of things compared to all the other changes coming), I'd propose dropping this hunk entirely, and revisiting when the merging is done... Warner
diff --git a/bsd-user/bsd-mman.h b/bsd-user/bsd-mman.h index 910e8c1921..5a64d0d425 100644 --- a/bsd-user/bsd-mman.h +++ b/bsd-user/bsd-mman.h @@ -1,4 +1,4 @@ -/*- +/* * Copyright (c) 1982, 1986, 1993 * The Regents of the University of California. All rights reserved. * @@ -30,16 +30,20 @@ * $FreeBSD: src/sys/sys/mman.h,v 1.42 2008/03/28 04:29:27 ps Exp $ */ -#define TARGET_FREEBSD_MAP_RESERVED0080 0x0080 /* previously misimplemented MAP_INHERIT */ -#define TARGET_FREEBSD_MAP_RESERVED0100 0x0100 /* previously unimplemented MAP_NOEXTEND */ -#define TARGET_FREEBSD_MAP_STACK 0x0400 /* region grows down, like a stack */ -#define TARGET_FREEBSD_MAP_NOSYNC 0x0800 /* page to but do not sync underlying file */ +#define TARGET_FREEBSD_MAP_RESERVED0080 0x0080 + /* previously misimplemented MAP_INHERIT */ +#define TARGET_FREEBSD_MAP_RESERVED0100 0x0100 + /* previously unimplemented MAP_NOEXTEND */ +#define TARGET_FREEBSD_MAP_STACK 0x0400 + /* region grows down, like a stack */ +#define TARGET_FREEBSD_MAP_NOSYNC 0x0800 + /* page to but do not sync underlying file */ #define TARGET_FREEBSD_MAP_FLAGMASK 0x1ff7 /* $NetBSD: mman.h,v 1.42 2008/11/18 22:13:49 ad Exp $ */ -/*- +/* * Copyright (c) 1982, 1986, 1993 * The Regents of the University of California. All rights reserved. * @@ -69,18 +73,21 @@ * * @(#)mman.h 8.2 (Berkeley) 1/9/95 */ -#define TARGET_NETBSD_MAP_INHERIT 0x0080 /* region is retained after exec */ -#define TARGET_NETBSD_MAP_TRYFIXED 0x0400 /* attempt hint address, even within break */ -#define TARGET_NETBSD_MAP_WIRED 0x0800 /* mlock() mapping when it is established */ - -#define TARGET_NETBSD_MAP_STACK 0x2000 /* allocated from memory, swap space (stack) */ +#define TARGET_NETBSD_MAP_INHERIT 0x0080 + /* region is retained after exec */ +#define TARGET_NETBSD_MAP_TRYFIXED 0x0400 + /* attempt hint address, even within break */ +#define TARGET_NETBSD_MAP_WIRED 0x0800 + /* mlock() mapping when it is established */ +#define TARGET_NETBSD_MAP_STACK 0x2000 + /* allocated from memory, swap space (stack) */ #define TARGET_NETBSD_MAP_FLAGMASK 0x3ff7 /* $OpenBSD: mman.h,v 1.18 2003/07/21 22:52:19 tedu Exp $ */ /* $NetBSD: mman.h,v 1.11 1995/03/26 20:24:23 jtc Exp $ */ -/*- +/* * Copyright (c) 1982, 1986, 1993 * The Regents of the University of California. All rights reserved. * @@ -111,11 +118,14 @@ * @(#)mman.h 8.1 (Berkeley) 6/2/93 */ -#define TARGET_OPENBSD_MAP_INHERIT 0x0080 /* region is retained after exec */ -#define TARGET_OPENBSD_MAP_NOEXTEND 0x0100 /* for MAP_FILE, don't change file size */ -#define TARGET_OPENBSD_MAP_TRYFIXED 0x0400 /* attempt hint address, even within heap */ +#define TARGET_OPENBSD_MAP_INHERIT 0x0080 + /* region is retained after exec */ +#define TARGET_OPENBSD_MAP_NOEXTEND 0x0100 + /* for MAP_FILE, don't change file size */ +#define TARGET_OPENBSD_MAP_TRYFIXED 0x0400 + /* attempt hint address, even within heap */ #define TARGET_OPENBSD_MAP_FLAGMASK 0x17f7 -// XXX +/* XXX what to do in the future? */ #define TARGET_BSD_MAP_FLAGMASK 0x3ff7