qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: "Alex Bennée" <alex.bennee@linaro.org>
To: quintela@redhat.com
Cc: "Daniel P. Berrangé" <berrange@redhat.com>, qemu-devel@nongnu.org
Subject: Re: [PATCH] Change the default for Mixed declarations.
Date: Fri, 24 Mar 2023 17:56:46 +0000	[thread overview]
Message-ID: <87zg822fnm.fsf@linaro.org> (raw)
In-Reply-To: <87pm8ykq09.fsf@secure.mitica>


Juan Quintela <quintela@redhat.com> writes:

> Daniel P. Berrangé <berrange@redhat.com> wrote:
>> On Tue, Feb 14, 2023 at 05:07:38PM +0100, Juan Quintela wrote:
>>> Hi
>>> 
>>> I want to enter a discussion about changing the default of the style
>>> guide.
>>> 
>>> There are several reasons for that:
>>> - they exist since C99 (i.e. all supported compilers support them)
>>> - they eliminate the posibility of an unitialized variable.
>>
>> Actually they don't do that reliably. In fact, when combined
>> with usage of 'goto', they introduce uninitialized variables,
>> despite the declaration having an initialization present, and
>> thus actively mislead reviewers into thinking their code is
>> safe.
>
> Wait a minute.
> If you use goto, you are already in special rules.
>
> And don't get confused, I fully agree when using goto for two reasons:
> - performance
>   if you show that the code is x% faster when using goto, it is
>   justified.  It is even better if you send a bug report to gcc/clang,
>   but I will not opose that use.

I await a clear example in the context of QEMU - there is almost always
a better way to structure things.

> - code clearity
>   Some code (basically error paths) are clearer with goto that without
>   them.

Now we have g_auto* and lock guards we should encourage their use. goto
error_path is a relic of a simpler time ;-)

<snip>
>> IMHO if we are concerned about uninitialized variables then I think
>> a better approach is to add -ftrivial-auto-var-init=zero, which will
>> make the compiler initialize all variables to 0 if they lack an
>> explicit initializer. 
>
> I think this is a bad idea.
> If we want to "catch" unitialized variables, using something like:
>
> -ftrivial-auto-var-init=pattern sounds much saner.
>
> Obviously gcc is missing
>
> -ftrivial-auto-var-init=42

I think we could at least eat the runtime cost of
-ftrvial-auto-var-init=0xDEADBEEF for our --enable-debug builds.

>
> But well, no project is perfect.
>
> Later, Juan.


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro


  reply	other threads:[~2023-03-24 18:01 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-14 16:07 [PATCH] Change the default for Mixed declarations Juan Quintela
2023-03-23 19:00 ` Daniel P. Berrangé
2023-03-24  8:43   ` Philippe Mathieu-Daudé
2023-03-24 14:04   ` Alex Bennée
2023-03-24 17:39   ` Juan Quintela
2023-03-24 17:56     ` Alex Bennée [this message]
2023-03-27  9:12       ` Daniel P. Berrangé
2023-03-27 10:49       ` Markus Armbruster
2023-03-27  9:10     ` Daniel P. Berrangé
2023-03-27 10:45   ` Markus Armbruster

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87zg822fnm.fsf@linaro.org \
    --to=alex.bennee@linaro.org \
    --cc=berrange@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=quintela@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).