Laszlo Ersek
2023-May-24 05:39 UTC
[Libguestfs] [libnbd PATCH] maint: pick consistent spacing style for casts
On 5/24/23 04:12, Eric Blake wrote:> We are inconsistent on whether a cast operator should be separated > from its argument by a space. As a unary prefix operator, casts bind > with relatively tight precedence, where only postfix operations > (including function calls) come higher. We generally don't use a > space after other prefix operators (*, &, ~, and unary - tend to not > be followed by space; we are less consistent on !, but that would be a > separate patch). > > Furthermore, we've already decided (in commit b5101fbc) that we prefer > spaces before function invocations. At the time, discussion on the > list[1] pointed out that thanks to the complexity of C with its > context-sensitive parsing needing to know whether an identifier is a > type name, you would be able to tell the difference between a > 2-argument function call with two parameters through a function > pointer (or intentional function-like macro suppression): > > (foo) (bar, baz) > > from the (admittedly rare) application of a cast operator to the > results of a comma operator: > > (foo)(bar, baz) > > if we consistently avoid spaces after casts. > > Determining cast operators is not always trivial, but the regex used > below seems to have a pretty low false positive rate (lines it selects > usually are casts and not in a comment line), and can be modified by > removing the space to see where we already had casts without a > space. (Obviously, I can't tell how many false negatives there are of > casts I missed out on). While this changes more lines than would be > done by instead always using a space in a cast, it is still manageable > to do a bulk change. > > $ git grep '\(_t\|int\|long\|signed\|char\|\*\)) [a-zA-Z0-9&"]' -- \ > '**/*.[hc]' '**/*.ml' | grep -v '^ */\?\*' > > 'git show -w' shows that this change is whitespace only. > > [1] https://listman.redhat.com/archives/libguestfs/2023-February/030771.html > > Signed-off-by: Eric Blake <eblake at redhat.com> > --- > configure.ac | 6 ++-- > common/utils/vector.h | 4 +-- > generator/OCaml.ml | 4 +-- > generator/Python.ml | 4 +-- > generator/states-connect-socket-activation.c | 4 +-- > generator/states-connect.c | 2 +- > generator/states-issue-command.c | 2 +- > generator/states-newstyle.c | 2 +- > generator/states-oldstyle.c | 2 +- > generator/states-reply-structured.c | 14 ++++---- > generator/states-reply.c | 2 +- > generator/states.c | 4 +-- > lib/crypto.c | 6 ++-- > lib/rw.c | 2 +- > lib/uri.c | 10 +++--- > lib/utils.c | 4 +-- > common/utils/test-vector.c | 4 +-- > python/handle.c | 4 +-- > python/utils.c | 2 +- > ocaml/nbd-c.h | 6 ++-- > tests/aio-connect-port.c | 2 +- > tests/aio-connect.c | 2 +- > tests/errors-bad-cookie.c | 2 +- > tests/errors-client-oversize.c | 2 +- > tests/errors-client-unadvertised-cmd.c | 2 +- > tests/errors-client-unaligned.c | 2 +- > tests/errors-client-unknown-flags.c | 2 +- > tests/errors-client-zerosize.c | 2 +- > tests/errors-connect-null.c | 2 +- > tests/errors-connect-twice.c | 4 +-- > tests/errors-multiple-disconnects.c | 2 +- > tests/errors-not-negotiating.c | 2 +- > tests/errors-notify-not-blocked.c | 2 +- > tests/errors-pread-structured.c | 2 +- > tests/errors-server-invalid-offset.c | 2 +- > tests/errors-server-oversize.c | 2 +- > tests/errors-server-unadvertised-cmd.c | 2 +- > tests/errors-server-unaligned.c | 2 +- > tests/errors-server-unknown-flags.c | 2 +- > tests/errors-server-zerosize.c | 2 +- > tests/opt-set-meta.c | 2 +- > tests/opt-starttls.c | 4 +-- > tests/opt-structured-twice.c | 2 +- > tests/pread-initialize.c | 2 +- > tests/private-data.c | 4 +-- > tests/server-death.c | 2 +- > tests/shutdown-flags.c | 2 +- > examples/glib-main-loop.c | 36 ++++++++++---------- > examples/open-qcow2.c | 2 +- > interop/list-exports.c | 2 +- > copy/file-ops.c | 6 ++-- > copy/main.c | 10 +++--- > copy/nbd-ops.c | 36 ++++++++++---------- > copy/pipe-ops.c | 8 ++--- > dump/dump.c | 2 +- > fuse/nbdfuse.c | 8 ++--- > fuse/operations.c | 10 +++--- > ublk/nbdublk.c | 4 +-- > ublk/tgt.c | 4 +-- > 59 files changed, 138 insertions(+), 138 deletions(-)Going through the changes individually, it seems like we could eliminate a handful of the casts altogether; for examle (char *)"string literal" ones. (The C standard effectively says that a (non-wide) string literal has type "static char[n]", not "static const char[n]".) But that would be a different patch, plus I can imagine we have those casts in the first place because gcc complained or whatever. :) Reviewed-by: Laszlo Ersek <lersek at redhat.com>
Richard W.M. Jones
2023-May-24 13:09 UTC
[Libguestfs] [libnbd PATCH] maint: pick consistent spacing style for casts
On Wed, May 24, 2023 at 07:39:20AM +0200, Laszlo Ersek wrote:> Going through the changes individually, it seems like we could eliminate > a handful of the casts altogether; for examle (char *)"string literal" > ones.It'd be interesting to remove then and compile with './configure --enable-gcc-warnings' to see if that creates new warnings.> (The C standard effectively says that a (non-wide) string literal > has type "static char[n]", not "static const char[n]".) But that > would be a different patch, plus I can imagine we have those casts > in the first place because gcc complained or whatever. :)It's a bit surprising. Surely any string literal is placed in the text section so definitely not writable? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
Eric Blake
2023-May-24 15:02 UTC
[Libguestfs] [libnbd PATCH] maint: pick consistent spacing style for casts
On Wed, May 24, 2023 at 07:39:20AM +0200, Laszlo Ersek wrote:> On 5/24/23 04:12, Eric Blake wrote: > > We are inconsistent on whether a cast operator should be separated > > from its argument by a space. As a unary prefix operator, casts bind > > with relatively tight precedence, where only postfix operations > > (including function calls) come higher. We generally don't use a > > space after other prefix operators (*, &, ~, and unary - tend to not > > be followed by space; we are less consistent on !, but that would be a > > separate patch). > > > > Going through the changes individually, it seems like we could eliminate > a handful of the casts altogether; for examle (char *)"string literal" > ones. (The C standard effectively says that a (non-wide) string literal > has type "static char[n]", not "static const char[n]".) But that would > be a different patch, plus I can imagine we have those casts in the > first place because gcc complained or whatever. :) > > Reviewed-by: Laszlo Ersek <lersek at redhat.com>Indeed, I did a manual review, and posted a followup patch of several casts that can be eliminated without causing any compiler warnings. I'm still planning on committing this patch as-is (whitespace only changes) but will wait until we review that patch to make sure I'm not eliminating a cast that would otherwise serve a useful documentation purpose despite being semantically unneeded. I could also rearrange the two patches (remove unneeded casts first, then fix whitespacing on the casts that remain) if desired. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org