Richard W.M. Jones
2023-Apr-19 10:53 UTC
[Libguestfs] [libnbd PATCH 08/18] lib/internal.h: wrap source code at 80 characters
On Tue, Apr 18, 2023 at 07:26:21PM +0200, Laszlo Ersek wrote:> (The changes in this patch are simple, but likely more controversial than > the rest.) > > The following four components don't play nice together: > > - Needlessly spelling out "extern" for function declarations in header > files. (C99 6.2.2p5: "If the declaration of an identifier for a function > has no storage-class specifier, its linkage is determined exactly as if > it were declared with the storage-class specifier extern [...]".)Well I didn't know that ...> - Long return type names. > > - Very long function names, such as "nbd_internal" + > ("crypto_create_session" or "state_group_parent"). > > - Not placing function names seen in declarators (as opposed to those seen > in definitions) at column #0, lest we confuse utilities that scan for > "tags". > > Shorten lines by breaking the function names to new lines as well, but > indent them to column #2.As long as the first column doesn't have the function name, so I can still 'git grep ^function' to find the definition, it's fine. And that seems to be the case here. Rich.> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2172516 > Signed-off-by: Laszlo Ersek <lersek at redhat.com> > --- > lib/internal.h | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/lib/internal.h b/lib/internal.h > index 2de8e4e5e043..b155681d057f 100644 > --- a/lib/internal.h > +++ b/lib/internal.h > @@ -407,7 +407,9 @@ extern int nbd_internal_wait_until_connected (struct nbd_handle *h) > LIBNBD_ATTRIBUTE_NONNULL (1); > > /* crypto.c */ > -extern struct socket *nbd_internal_crypto_create_session (struct nbd_handle *, struct socket *oldsock) > +extern struct socket * > + nbd_internal_crypto_create_session (struct nbd_handle *, > + struct socket *oldsock) > LIBNBD_ATTRIBUTE_NONNULL (1, 2); > extern bool nbd_internal_crypto_is_reading (struct nbd_handle *) > LIBNBD_ATTRIBUTE_NONNULL (1); > @@ -504,7 +506,8 @@ extern int nbd_internal_run (struct nbd_handle *h, enum external_event ev) > LIBNBD_ATTRIBUTE_NONNULL (1); > extern const char *nbd_internal_state_short_string (enum state state); > extern enum state_group nbd_internal_state_group (enum state state); > -extern enum state_group nbd_internal_state_group_parent (enum state_group group); > +extern enum state_group > + nbd_internal_state_group_parent (enum state_group group); > extern int nbd_internal_aio_get_direction (enum state state); > > #define set_next_state(h,next_state) ((h)->state) = (next_state) > > _______________________________________________ > Libguestfs mailing list > Libguestfs at redhat.com > https://listman.redhat.com/mailman/listinfo/libguestfs-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
Eric Blake
2023-Apr-19 13:19 UTC
[Libguestfs] [libnbd PATCH 08/18] lib/internal.h: wrap source code at 80 characters
On Wed, Apr 19, 2023 at 11:53:44AM +0100, Richard W.M. Jones wrote:> On Tue, Apr 18, 2023 at 07:26:21PM +0200, Laszlo Ersek wrote: > > (The changes in this patch are simple, but likely more controversial than > > the rest.) > > > > The following four components don't play nice together: > > > > - Needlessly spelling out "extern" for function declarations in header > > files. (C99 6.2.2p5: "If the declaration of an identifier for a function > > has no storage-class specifier, its linkage is determined exactly as if > > it were declared with the storage-class specifier extern [...]".) > > Well I didn't know that ...https://stackoverflow.com/questions/18171373/why-are-some-functions-declared-extern-and-header-file-not-included-in-source-in/18173554#18173554 gives some more interesting details (including what happens if you declare a function more than once, using different specifiers across the declarations). I'm okay with dropping extern on functions if you want, but that can be a separate patch; leaving them doesn't hurt either (other than the line length issues where your line splitting techniques are fine).> > +++ b/lib/internal.h > > @@ -407,7 +407,9 @@ extern int nbd_internal_wait_until_connected (struct nbd_handle *h) > > LIBNBD_ATTRIBUTE_NONNULL (1); > > > > /* crypto.c */ > > -extern struct socket *nbd_internal_crypto_create_session (struct nbd_handle *, struct socket *oldsock) > > +extern struct socket * > > + nbd_internal_crypto_create_session (struct nbd_handle *, > > + struct socket *oldsock) > > LIBNBD_ATTRIBUTE_NONNULL (1, 2);Looks reasonable to me. emacs may have a bit of a difficulty in auto-indenting this back to the same place, but it is infrequent enough that manual override is still viable. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Laszlo Ersek
2023-Apr-19 13:23 UTC
[Libguestfs] [libnbd PATCH 08/18] lib/internal.h: wrap source code at 80 characters
On 4/19/23 15:19, Eric Blake wrote:> On Wed, Apr 19, 2023 at 11:53:44AM +0100, Richard W.M. Jones wrote: >> On Tue, Apr 18, 2023 at 07:26:21PM +0200, Laszlo Ersek wrote: >>> (The changes in this patch are simple, but likely more controversial than >>> the rest.) >>> >>> The following four components don't play nice together: >>> >>> - Needlessly spelling out "extern" for function declarations in header >>> files. (C99 6.2.2p5: "If the declaration of an identifier for a function >>> has no storage-class specifier, its linkage is determined exactly as if >>> it were declared with the storage-class specifier extern [...]".) >> >> Well I didn't know that ... > > https://stackoverflow.com/questions/18171373/why-are-some-functions-declared-extern-and-header-file-not-included-in-source-in/18173554#18173554 > > gives some more interesting details (including what happens if you > declare a function more than once, using different specifiers across > the declarations).In 2009, I constructed the following table, and I've been using it since: ----------------- Name space: ordinary identifiers Declaration | Scope | Linkage | Storage duration | Definition ------------------------+-------+--------------------------------------------------------------------+------------------------+-------------------------- int obj_1; | file | external ((6.1.2.2 p5) | static (6.1.2.4 p2) | tentative (6.7.2 p2) extern int obj_2; | file | same as any visible file scope declaration / external (6.1.2.2 p4) | static (6.1.2.4 p2)* | none (6.7.2) static int obj_3; | file | internal (6.1.2.2 p3) | static (6.1.2.4 p2) | tentative (6.7.2 p2) int obj_4 = 1; | file | external ((6.1.2.2 p5) | static (6.1.2.4 p2) | external (6.7.2 p1) extern int obj_5 = 1; | file | same as any visible file scope declaration / external (6.1.2.2 p4) | static (6.1.2.4 p2)* | external (6.7.2 p1) static int obj_6 = 1; | file | internal (6.1.2.2 p3) | static (6.1.2.4 p2) | external (6.7.2 p1) int fun_1(void); | file | same as any visible file scope declaration / external (6.1.2.2 p5) | n/a (6.1.2.4) | none (6.5 p4 fn56, 6.7.1) extern int fun_2(void); | file | same as any visible file scope declaration / external (6.1.2.2 p4) | n/a (6.1.2.4) | none (6.5 p4 fn56, 6.7.1) static int fun_3(void); | file | internal (6.1.2.2 p3) | n/a (6.1.2.4) | none (6.5 p4 fn56, 6.7.1) ------------------------+-------+--------------------------------------------------------------------+------------------------+-------------------------- int obj_7; | block | none (6.1.2.2 p6) | automatic (6.1.2.4 p3) | yes (6.1.2.4 p3, 6.5 p4) extern int obj_8; | block | same as any visible file scope declaration / external (6.1.2.2 p4) | static (6.1.2.4 p2)* | none (6.7.2) static int obj_9; | block | none (6.1.2.2 p6) | static (6.1.2.4 p2) | yes (6.1.2.4 p2, 6.5 p4) int obj_10 = 1; | block | none (6.1.2.2 p6) | automatic (6.1.2.4 p3) | yes (6.1.2.4 p3, 6.5 p4) extern int obj_11 = 1; | block | INVALID (6.5.7 p4)** | INVALID | INVALID static int obj_12 = 1; | block | none (6.1.2.2 p6) | static (6.1.2.4 p2) | yes (6.1.2.4 p2, 6.5 p4) int fun_4(void); | block | same as any visible file scope declaration / external (6.1.2.2 p5) | n/a (6.1.2.4) | none (6.5 p4 fn56, 6.7.1) extern int fun_5(void); | block | same as any visible file scope declaration / external (6.1.2.2 p4) | n/a (6.1.2.4) | none (6.5 p4 fn56, 6.7.1) static int fun_6(void); | block | INVALID (6.1.2.2 p3 fn13, 6.5.1 p4) | INVALID | INVALID * any visible file scope declaration can only specify either external or internal linkage ** such a declaration would specify "same as any visible file scope declaration / external (6.1.2.2 p4)" linkage, that is, external or internal (see *) ----------------- All the references are to C89, unfortunately. I've never gotten around updating them to C99. :/> > I'm okay with dropping extern on functions if you want, but that can > be a separate patch; leaving them doesn't hurt either (other than the > line length issues where your line splitting techniques are fine).Removing the "extern"s is too much churn :) We use them very consistently, and they are not "wrong" by any means. They're a bit uncomfortable in this case, but if the current patch is tolerable, I'd prefer it over a global de-extern-ing.> >>> +++ b/lib/internal.h >>> @@ -407,7 +407,9 @@ extern int nbd_internal_wait_until_connected (struct nbd_handle *h) >>> LIBNBD_ATTRIBUTE_NONNULL (1); >>> >>> /* crypto.c */ >>> -extern struct socket *nbd_internal_crypto_create_session (struct nbd_handle *, struct socket *oldsock) >>> +extern struct socket * >>> + nbd_internal_crypto_create_session (struct nbd_handle *, >>> + struct socket *oldsock) >>> LIBNBD_ATTRIBUTE_NONNULL (1, 2); > > Looks reasonable to me. emacs may have a bit of a difficulty in > auto-indenting this back to the same place, but it is infrequent > enough that manual override is still viable. >Thanks! I'll stick with this, then. Laszlo