Date: Thu, 19 Dec 2013 16:20:20 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Adrian Chadd <adrian@FreeBSD.org> Cc: "freebsd-arch@freebsd.org" <freebsd-arch@FreeBSD.org> Subject: Re: Using sys/types.h types in sys/socket.h Message-ID: <20131219154839.T23018@besplex.bde.org> In-Reply-To: <CAJ-Vmom%2BXMZgdKds88id9vhQar=P-bF3UpUFzk4E3KWUw%2BQacQ@mail.gmail.com> References: <CAJ-Vmo=MWPQWfP9duWPPwaKee5Zp9Gemj3GKqE8=bxkjn_1YYA@mail.gmail.com> <9C1291B5-215B-440E-B8B0-6308840F755C@bsdimp.com> <CAJ-Vmokb-gcO%2BrEOn-uc42%2BPHzMMQsqBe0NcVtuNRKk7vuM5Qw@mail.gmail.com> <CAJ-Vmom%2BXMZgdKds88id9vhQar=P-bF3UpUFzk4E3KWUw%2BQacQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 18 Dec 2013, Adrian Chadd wrote:
> Ok, how about this:
>
> Index: sys/sys/socket.h
> ===================================================================
> --- sys/sys/socket.h (revision 259475)
> +++ sys/sys/socket.h (working copy)
> @@ -84,6 +84,16 @@
> #endif
> #endif
>
> +#ifndef _UINT32_T_DECLARED
> +#define _UINT32_T_DECLARED
> +typedef __uint32_t uint32_t;
> +#endif
> +
> +#ifndef _UINTPTR_T_DECLARED
> +#define _UINTPTR_T_DECLARED
> +typedef __uintptr_t uintptr_t;
> +#endif
> +
> /*
> * Types
> */
This seems to be correct, except the tab after the second #define is
corrupt. Actually, all the tabs are corrupt, but the first #define
apparently started with a tab whose corruption made a larger mess.
imp@ said, in a message that should have been killfiled due to top posting,
that this should be under __BSD_VISIBLE. That isn't strictly necessary,
since POSIX allows names ending with _t, and it isn't very important for
avoiding pollution since there aren't very many of them.
> @@ -577,11 +587,27 @@
> };
>
> /*
> + * sendfile(2) kqueue information
> + */
> +struct sf_hdtr_kq {
> + int kq_fd; /* kq fd to post completion events on */
> + int kq_fd; /* kq fd to post completion events on */
> + uint32_t kq_flags; /* extra flags to pass in */
> + void *kq_udata; /* user data pointer */
> + uintptr_t kq_ident; /* ident (from userland?) */
> +};
kq_fd is duplicated.
All of the indentation is wrong and has corrupt tabs, except the first
level only has corrupt tabs.
This should be:
struct sf_hdtr_kq {
int kq_fd; /* kq fd to post completion events on */
uint32_t kq_flags; /* extra flags to pass in */
void *kq_udata; /* user data pointer */
uintptr_t kq_ident; /* ident (from userland?) */
};
I strongly disagree with indenting everything N extra levels (where N > 1;
N = 2 here for the name fields) to line up the fields. I disagree with
indenting the name fields by 1 space extra to line them up after adding
a '*' before a few fields. It's hard enough to keep the style consistent
without following this fancy style.
> +
> +struct sf_hdtr_all {
> + struct sf_hdtr hdtr;
> + struct sf_hdtr_kq kq;
> +};
Here an extra level would be reasonable since all fields need it, except
it would leave less space for comments. Comments are needed for these
fields more than for most, since there are none for the struct itself.
> +
> +/*
> * Sendfile-specific flag(s)
> */
Older style bugs:
- sendfile(2) is not spelled consistently
- the parentheses around 's' are more bogus than before because it is
even more obvious that there is more than 1 flag here. This was
non-bogus in FreeBSD-5 where the only flag here was SF_NODISKIO.
- this and some other comments are too verbose (block comments are not needed)
- the style rules for block comments are stricter, but are not followed.
The sentence fragment in this comment is not a full sentence, and the
paragraph in this comment is not filled to make it look like a real
paragraph (filling would also make the sentence fragment look like a
sentence). I like to write comments about flags uniformly as
"/* Flags for foo: */".
> #define SF_NODISKIO 0x00000001
> #define SF_MNOWAIT 0x00000002
> #define SF_SYNC 0x00000004
> +#define SF_KQUEUE 0x00000008
The names are unsorted. This seems to be unfortunately necessary for binary
compatibility.
>
This blank line breaks the scope of the comment.
> #ifdef _KERNEL
> #define SFK_COMPAT 0x00000001
>
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20131219154839.T23018>