Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts
On Wed, May 30, 2012 at 3:26 AM, Anthony Liguori aligu...@us.ibm.com wrote: 3) It's not how the rest of QEMU is written. Consistency is the most important purpose of Coding Style. (3) is the most important consideration of all. Fair enough if its a style choice and you want QEMU to be consistent. I'd love to use them because they keep variables and the code that uses them together - great for checking that correct types are being used during code review and also less noise in the patch. So if you ever change your mind about this, let me know and I'll never declare a variable at the start of a function again :D. Stefan
Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts
On 30 May 2012 08:33, Stefan Hajnoczi stefa...@gmail.com wrote:
I'd love to use them because they keep variables and the code that
uses them together - great for checking that correct types are being
used during code review and also less noise in the patch.
Just open a new scope with { and close it when you're done :-)
-- PMM
Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts
On Wed, May 30, 2012 at 8:34 AM, Peter Maydell peter.mayd...@linaro.org wrote:
On 30 May 2012 08:33, Stefan Hajnoczi stefa...@gmail.com wrote:
I'd love to use them because they keep variables and the code that
uses them together - great for checking that correct types are being
used during code review and also less noise in the patch.
Just open a new scope with { and close it when you're done :-)
{Thanks {for {your {advice {soon {I'll {be {writing {LISP}
Stefan
Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts
Am 30.05.2012 09:33, schrieb Stefan Hajnoczi: On Wed, May 30, 2012 at 3:26 AM, Anthony Liguori aligu...@us.ibm.com wrote: 3) It's not how the rest of QEMU is written. Consistency is the most important purpose of Coding Style. (3) is the most important consideration of all. Fair enough if its a style choice and you want QEMU to be consistent. I'd love to use them because they keep variables and the code that uses them together - great for checking that correct types are being used during code review and also less noise in the patch. So if you ever change your mind about this, let me know and I'll never declare a variable at the start of a function again :D. You would have to find ways to bypass the block maintainer. ;-) I generally think it's good style to keep declarations together at the top of a block. Except sometimes. (And in some cases like VLAs it can even be necessary to have them in the middle of a function) Kevin
Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts
On 05/28/2012 08:39 PM, Stefan Hajnoczi wrote:
On Wed, May 23, 2012 at 5:03 PM, Stefan Weils...@weilnetz.de wrote:
Am 23.05.2012 17:32, schrieb Kevin Wolf:
Maybe, but I already had patches rejected because of that style.
Did this policy change? I'd appreciate that!
Agreed, people have been asked to declare variables at the beginning
of the scope. I don't understand why, C99 allows them to be declared
anywhere and it usually makes the code more readable IMO (you don't
have to jump to the definition to check a variable's type).
What's the problem with C99-style variable declarations anywhere in a function?
That was a dumb additional to C99 to help with C++ compatibility. The only
reason it was added to C++ is because constructors can have side effects and if
you want to do RAII, you really need to defer construction to right before you
want to acquire the resource. IOW, if you want to do:
void foo(void)
{
MyDataStructure *s;
s = find_data_structure();
// do non-trivial things to find s if it's NULL
ScopedLock lock(s-lock);
// more stuff holding s's lock
}
You have no choice but to do it this way. RAII is the only reasonable way to
deal with exceptions too since the destructor for the lock will be called in the
exception path (to release the mutex on error).
This is why C++ *had* to support mixing type declarations with code. Now here's
why doing it in C is a really bad idea:
1) If you're looking at a complex function, it becomes difficult to find where
the variable is declared. This can create interesting problems with shadowing.
2) It encourages having overly complicated functions. An advantage of putting
variables at the top is that you quickly see when a function has way too many
variables in it. Sprinkling the variable declarations through the function
makes it far too easy to create monster functions.
3) It's not how the rest of QEMU is written. Consistency is the most important
purpose of Coding Style.
(3) is the most important consideration of all.
Regards,
Anthony Liguori
Stefan
Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts
On Wed, May 23, 2012 at 5:03 PM, Stefan Weil s...@weilnetz.de wrote: Am 23.05.2012 17:32, schrieb Kevin Wolf: Am 23.05.2012 17:29, schrieb Stefan Weil: Am 23.05.2012 10:09, schrieb Stefan Hajnoczi: On Tue, May 22, 2012 at 10:23 PM, Stefan Weils...@weilnetz.de wrote: The local variables ret, i are only used if __linux__ is defined. Signed-off-by: Stefan Weils...@weilnetz.de --- hw/virtio-blk.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) The #ifdef __linux__ further down in the function declares the local hdr variable. We could move ret and i down into the #ifdef instead of adding a new one. I noticed that, but declaring variables anywhere is C++, not C code. It's called C99. Kevin Maybe, but I already had patches rejected because of that style. Did this policy change? I'd appreciate that! Agreed, people have been asked to declare variables at the beginning of the scope. I don't understand why, C99 allows them to be declared anywhere and it usually makes the code more readable IMO (you don't have to jump to the definition to check a variable's type). What's the problem with C99-style variable declarations anywhere in a function? Stefan
Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts
On Mon, May 28, 2012 at 01:39:58PM +0100, Stefan Hajnoczi wrote:
On Wed, May 23, 2012 at 5:03 PM, Stefan Weil s...@weilnetz.de wrote:
Am 23.05.2012 17:32, schrieb Kevin Wolf:
Am 23.05.2012 17:29, schrieb Stefan Weil:
Am 23.05.2012 10:09, schrieb Stefan Hajnoczi:
On Tue, May 22, 2012 at 10:23 PM, Stefan Weils...@weilnetz.de wrote:
The local variables ret, i are only used if __linux__ is defined.
Signed-off-by: Stefan Weils...@weilnetz.de
---
hw/virtio-blk.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
The #ifdef __linux__ further down in the function declares the local
hdr variable. We could move ret and i down into the #ifdef instead of
adding a new one.
I noticed that, but declaring variables anywhere is C++, not C code.
It's called C99.
Kevin
Maybe, but I already had patches rejected because of that style.
Did this policy change? I'd appreciate that!
Agreed, people have been asked to declare variables at the beginning
of the scope. I don't understand why, C99 allows them to be declared
anywhere and it usually makes the code more readable IMO (you don't
have to jump to the definition to check a variable's type).
What's the problem with C99-style variable declarations anywhere in a
function?
Not an issue in this particular patch, but declaration at point of
first use can cause you problems if the code uses 'goto' alot (eg
for return cleanup).
eg
void somefunc(void) {
...some code...
if (...something bad...)
goto cleanup;
... some code...
char *bar = NULL;
bar = strdup(Hello World);
...some code...
cleanup:
free(bar);
}
In that 'cleanup' label 'bar' will potentially be uninitialized,
because it is in scope, but the line where initialization takes
place may never been reached.
So if you do decide to make use of C99 style decl at first use,
then be sure to turn on the GCC warnings to detect these variable
initialization problems.
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts
On 28 May 2012 13:39, Stefan Hajnoczi stefa...@gmail.com wrote: Agreed, people have been asked to declare variables at the beginning of the scope. I don't understand why, C99 allows them to be declared anywhere and it usually makes the code more readable IMO (you don't have to jump to the definition to check a variable's type). If the definition is that far away from the use it's probably a sign that your function is too large anyway and you should be looking for a smaller scope within which to declare the variable... What's the problem with C99-style variable declarations anywhere in a function? I think they look slightly uglier but that's purely a personal prejudice which I don't think I can justify imposing on anybody else :-) -- PMM
Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts
Am 23.05.2012 10:09, schrieb Stefan Hajnoczi: On Tue, May 22, 2012 at 10:23 PM, Stefan Weil s...@weilnetz.de wrote: The local variables ret, i are only used if __linux__ is defined. Signed-off-by: Stefan Weil s...@weilnetz.de --- hw/virtio-blk.c |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) The #ifdef __linux__ further down in the function declares the local hdr variable. We could move ret and i down into the #ifdef instead of adding a new one. Sorry, this patch seems to have passed me by: I posted a patch moving all Linux variables into an #ifdef at the top. http://patchwork.ozlabs.org/patch/161546/ Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts
On Tue, May 22, 2012 at 10:23 PM, Stefan Weil s...@weilnetz.de wrote: The local variables ret, i are only used if __linux__ is defined. Signed-off-by: Stefan Weil s...@weilnetz.de --- hw/virtio-blk.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) The #ifdef __linux__ further down in the function declares the local hdr variable. We could move ret and i down into the #ifdef instead of adding a new one.
Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts
Am 23.05.2012 10:09, schrieb Stefan Hajnoczi: On Tue, May 22, 2012 at 10:23 PM, Stefan Weils...@weilnetz.de wrote: The local variables ret, i are only used if __linux__ is defined. Signed-off-by: Stefan Weils...@weilnetz.de --- hw/virtio-blk.c |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) The #ifdef __linux__ further down in the function declares the local hdr variable. We could move ret and i down into the #ifdef instead of adding a new one. I noticed that, but declaring variables anywhere is C++, not C code. hdr violates the QEMU coding rules (other patches which did not declare local variables at the beginning of a block were already rejected). That's why I wrote the patch as it is. Regards, Stefan W.
Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts
Am 23.05.2012 17:29, schrieb Stefan Weil: Am 23.05.2012 10:09, schrieb Stefan Hajnoczi: On Tue, May 22, 2012 at 10:23 PM, Stefan Weils...@weilnetz.de wrote: The local variables ret, i are only used if __linux__ is defined. Signed-off-by: Stefan Weils...@weilnetz.de --- hw/virtio-blk.c |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) The #ifdef __linux__ further down in the function declares the local hdr variable. We could move ret and i down into the #ifdef instead of adding a new one. I noticed that, but declaring variables anywhere is C++, not C code. It's called C99. Kevin
Re: [Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts
Am 23.05.2012 17:32, schrieb Kevin Wolf: Am 23.05.2012 17:29, schrieb Stefan Weil: Am 23.05.2012 10:09, schrieb Stefan Hajnoczi: On Tue, May 22, 2012 at 10:23 PM, Stefan Weils...@weilnetz.de wrote: The local variables ret, i are only used if __linux__ is defined. Signed-off-by: Stefan Weils...@weilnetz.de --- hw/virtio-blk.c |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) The #ifdef __linux__ further down in the function declares the local hdr variable. We could move ret and i down into the #ifdef instead of adding a new one. I noticed that, but declaring variables anywhere is C++, not C code. It's called C99. Kevin Maybe, but I already had patches rejected because of that style. Did this policy change? I'd appreciate that! Stefan
[Qemu-devel] [PATCH 1.1] virtio: Fix compiler warning for non Linux hosts
The local variables ret, i are only used if __linux__ is defined.
Signed-off-by: Stefan Weil s...@weilnetz.de
---
hw/virtio-blk.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index f9e1896..60750cb 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -147,9 +147,11 @@ static VirtIOBlockReq *virtio_blk_get_request(VirtIOBlock
*s)
static void virtio_blk_handle_scsi(VirtIOBlockReq *req)
{
+#ifdef __linux__
int ret;
-int status = VIRTIO_BLK_S_OK;
int i;
+#endif
+int status = VIRTIO_BLK_S_OK;
/*
* We require at least one output segment each for the virtio_blk_outhdr
--
1.7.10
