Re: [Qemu-devel] [PATCH v2 31/40] job: Add job_is_ready()

[Kevin Wolf <kwolf@redhat.com>]

Am 24.05.2018 um 19:25 hat John Snow geschrieben:
> >>> diff --git a/job.c b/job.c
> >>> index af31de4669..66ee26f2a0 100644
> >>> --- a/job.c
> >>> +++ b/job.c
> >>> @@ -199,6 +199,28 @@ bool job_is_cancelled(Job *job)
> >>>      return job->cancelled;
> >>>  }
> >>>  
> >>> +bool job_is_ready(Job *job)
> >>> +{
> >>> +    switch (job->status) {
> >>> +    case JOB_STATUS_UNDEFINED:
> >>> +    case JOB_STATUS_CREATED:
> >>> +    case JOB_STATUS_RUNNING:
> >>> +    case JOB_STATUS_PAUSED:
> >>> +    case JOB_STATUS_WAITING:
> >>> +    case JOB_STATUS_PENDING:
> >>> +    case JOB_STATUS_ABORTING:
> >>> +    case JOB_STATUS_CONCLUDED:
> >>> +    case JOB_STATUS_NULL:
> >>> +        return false;
> >>> +    case JOB_STATUS_READY:
> >>> +    case JOB_STATUS_STANDBY:
> >>> +        return true;
> >>> +    default:
> >>> +        g_assert_not_reached();
> >>> +    }
> >>> +    return false;
> >>> +}
> >>> +
> >>
> >> What's the benefit to a switch statement with a default clause here,
> >> over the shorter:
> >>
> >> if (job->status == READY || job->status == STANDBY) {
> >>   return true;
> >> }
> >> return false;
> >>
> >> (Yes, I realize you already merged this code, but I'm still curious and
> >> I need to read the series anyway to see what's changed...)
> > 
> > That it's easy to copy and paste from job_is_completed()? :-P
> > 
> 
> Haha! Sure!
> 
> > I guess you could argue that the switch ensures that we don't forget to
> > explicitly handle every state if we ever add a new one, but the real
> > reason is more like, job_is_completed() was already there and I didn't
> > see a reason to do something different here.
> > 
> 
> I think the "default" case removes that benefit somewhat; it's nicer
> when the compiler yelps at you for forgetting. The cases that might
> cause an assertion could be harder to hit.

Good point. I'm not sure which compilers would actually warn here, while
asserting makes sure to crash some tests, but in this context I think we
could actually have both: All enum values that are handled return inside
the switch statement. So if we move the g_assert_not_reached() just
after the switch block, we should get both the compiler warning and the
crash.

Would you like to send a patch?

Kevin