[Spice-devel] [server patch v2] test-loop: increment a variable outside of spice_assert

[Frediano Ziglio]

> 
> Hi,
> 
> On Thu, Aug 22, 2019 at 06:53:12PM +0300, Uri Lublin wrote:
> > spice_assert is a macro and covscan reports that:
> >   Argument "++twice_remove_called" of spice_assert() has a side effect.
> > 
> > Doesn't matter if there is a side effects or not,
> > it's a good practice and it makes covscan happy, so
> > increment the variable one line above.
> > 
> > Signed-off-by: Uri Lublin <uril at redhat.com>
> > ---
> > 
> > v1->v2: change commit log (Frediano)
> > 
> > ---
> >  server/tests/test-loop.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/server/tests/test-loop.c b/server/tests/test-loop.c
> > index 82af80ab3..4df3a7d20 100644
> > --- a/server/tests/test-loop.c
> > +++ b/server/tests/test-loop.c
> > @@ -78,7 +78,8 @@ static SpiceTimer *twice_timers_remove[2] = { NULL, NULL
> > };
> >  static int twice_remove_called = 0;
> >  static void timer_not_twice_remove(void *opaque)
> >  {
> > -    spice_assert(++twice_remove_called == 1);
> > +    ++twice_remove_called;
> 
> Silly question here but is there a reason why pre-increment would
> be preferred in this case?
> 

It's not silly. I think from my side coming from a past C++ and knowing
there's no much difference in this case from post and pre I didn't pay much
attention (not much important), patch was fine.
I suppose the change is minor as previously was pre increment and so
this was preserved (in previous code pre and post mattered).
And personally these tools are sometimes useful but after you remove the
main things what's left are mostly false positive which are more boring
than helpful. Not that they don't do a good job, it's just math, if they
were 95% accurate (they are much less, Coverity stated a 60% in the past)
and you removed that 95% what's left are false positive so next time probably
there will be 95% false positive.

> > +    spice_assert(twice_remove_called == 1);
> >  
> >      /* delete timers, should not have another call */
> >      core->timer_remove(twice_timers_remove[0]);

Frediano