> > On 19 Mar 2018, at 14:46, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> >
> > Allow to enable code to do additional or expensive checks.
>
> “Allow to enable…” -> “Add configuration option enabling expensive checks”
>
We decided extra after researching on different projects.
I think extra is better, not only expensive but also paranoid tests
I would say that you don't want on final product.
> > The option should be used by higher level libraries.
> > By default the option is disabled.
> >
> > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > ---
> > common/log.h | 6 ++++++
> > configure.ac | 1 +
> > m4/spice-deps.m4 | 14 ++++++++++++++
> > 3 files changed, 21 insertions(+)
> >
> > diff --git a/common/log.h b/common/log.h
> > index 06d48d2..e0fd34b 100644
> > --- a/common/log.h
> > +++ b/common/log.h
> > @@ -93,6 +93,12 @@ void spice_log(GLogLevelFlags log_level,
> > } \
> > } G_STMT_END
> >
> > +#if ENABLE_EXTRA_CHECKS
> > +enum { spice_extra_checks = 1 };
>
> I would suggest renaming “extra_checks” to “expensive_checks” if that’s
> really the intent.
No, is extra.
> And then, I would avoid bracketing inexpensive checks (like simple asserts)
> with it, because otherwise it introduces confusion.
>
I don't see why an if is confusing. They are "extra", not "expensive".
> Alternatively, if we think that asserts are expensive, then all spice_assert
> checks should be disabled by this. But I’d rather not.
>
> (I already suggested a spice_hot_assert for assertions in hot code as a
> better way to express the intent than an if around spice_assert).
>
I don't agree on a rule like "is expensive disable the assert", depends
on the probability of happening and the problem that can raise not doing
it. I prefer a core than a remote code execution, but here we enter on
the opinions.
> > +#else
> > +enum { spice_extra_checks = 0 };
> > +#endif
> > +
> > SPICE_END_DECLS
> >
> > #endif /* H_SPICE_LOG */
> > diff --git a/configure.ac b/configure.ac
> > index 3542161..3da85de 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -27,6 +27,7 @@ fi
> > AM_PROG_CC_C_O
> >
> > SPICE_CHECK_SYSDEPS
> > +SPICE_EXTRA_CHECKS
> >
> > AC_ARG_ENABLE([alignment-checks],
> > AS_HELP_STRING([--enable-alignment-checks],
> > diff --git a/m4/spice-deps.m4 b/m4/spice-deps.m4
> > index 68e3091..a6f4b7b 100644
> > --- a/m4/spice-deps.m4
> > +++ b/m4/spice-deps.m4
> > @@ -23,6 +23,20 @@ AC_DEFUN([SPICE_PRINT_MESSAGES],[
> > ])
> >
> >
> > +# SPICE_EXTRA_CHECKS()
> > +# --------------------
> > +# Check for --enable-extra-checks option
> > +# --------------------
> > +AC_DEFUN([SPICE_EXTRA_CHECKS],[
> > +AC_ARG_ENABLE([extra-checks],
> > + AS_HELP_STRING([--enable-extra-checks=@<:@yes/no@:>@],
> > + [Enable expensive checks
> > @<:@default=no@:>@]))
> > +AM_CONDITIONAL(ENABLE_EXTRA_CHECKS, test "x$enable_extra_checks" = "xyes")
> > +AS_IF([test "x$enable_extra_checks" = "xyes"],
> > + [AC_DEFINE([ENABLE_EXTRA_CHECKS], 1, [Enable extra checks on
> > code])])
> > +])
> > +
> > +
> > # SPICE_CHECK_SYSDEPS()
> > # ---------------------
> > # Checks for header files and library functions needed by spice-common.
Here comments would be better using "dnl" style but I used shell comments
for coherence.
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel