| Message ID | 20200902080801.160652-6-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | misc: Reduce assert side-effects | expand |
On Wed, 2 Sep 2020 10:08:01 +0200 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > This variable is used once in an assertion. Remove single > declaration and access directly in the assert(). > > See in "qemu/osdep.h": > > * [...] disable assertion is not > * supported upstream so the risk is all yours. Meanwhile, please > * submit patches to remove any side-effects inside an assertion, or > * fixing error handling that should use Error instead of assert. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > util/qsp.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/util/qsp.c b/util/qsp.c > index 7d5147f1b20..1db044ecedd 100644 > --- a/util/qsp.c > +++ b/util/qsp.c > @@ -560,9 +560,7 @@ static void qsp_iter_diff(void *p, uint32_t hash, void *htp) > > /* No point in reporting an empty entry */ > if (new->n_acqs == 0 && new->ns == 0) { > - bool removed = qht_remove(ht, new, hash); > - > - g_assert(removed); > + g_assert(qht_remove(ht, new, hash)); Urgh... this is doing exactly the opposite of the "qemu/osdep.h" recommandations above. We still want to remove new from the hash table even if QEMU was built without assertions. > g_free(new); > } > }
On 9/2/20 11:11 AM, Greg Kurz wrote: > On Wed, 2 Sep 2020 10:08:01 +0200 > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >> This variable is used once in an assertion. Remove single >> declaration and access directly in the assert(). >> >> See in "qemu/osdep.h": >> >> * [...] disable assertion is not >> * supported upstream so the risk is all yours. Meanwhile, please >> * submit patches to remove any side-effects inside an assertion, or >> * fixing error handling that should use Error instead of assert. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> util/qsp.c | 4 +--- >> 1 file changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/util/qsp.c b/util/qsp.c >> index 7d5147f1b20..1db044ecedd 100644 >> --- a/util/qsp.c >> +++ b/util/qsp.c >> @@ -560,9 +560,7 @@ static void qsp_iter_diff(void *p, uint32_t hash, void *htp) >> >> /* No point in reporting an empty entry */ >> if (new->n_acqs == 0 && new->ns == 0) { >> - bool removed = qht_remove(ht, new, hash); >> - >> - g_assert(removed); >> + g_assert(qht_remove(ht, new, hash)); > > Urgh... this is doing exactly the opposite of the "qemu/osdep.h" > recommandations above. We still want to remove new from the > hash table even if QEMU was built without assertions. Oops you are right :/ > >> g_free(new); >> } >> } >
diff --git a/util/qsp.c b/util/qsp.c index 7d5147f1b20..1db044ecedd 100644 --- a/util/qsp.c +++ b/util/qsp.c @@ -560,9 +560,7 @@ static void qsp_iter_diff(void *p, uint32_t hash, void *htp) /* No point in reporting an empty entry */ if (new->n_acqs == 0 && new->ns == 0) { - bool removed = qht_remove(ht, new, hash); - - g_assert(removed); + g_assert(qht_remove(ht, new, hash)); g_free(new); } }
This variable is used once in an assertion. Remove single declaration and access directly in the assert(). See in "qemu/osdep.h": * [...] disable assertion is not * supported upstream so the risk is all yours. Meanwhile, please * submit patches to remove any side-effects inside an assertion, or * fixing error handling that should use Error instead of assert. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- util/qsp.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)