I'm not sure the null check was in place for the safety of deletion so much
as to make the code easier to reason about:
if (process != NULL) {
terminate(process);
wait(process);
delete process;
}
if (credential != NULL) {
delete credential;
}
delete latch;
Here, (1) there's sometimes a process, (2) theres's sometimes a credential,
(3) there's always a latch. Without the null check for credential, the
optionality seems less clear to the reader (more non-local reasoning
required). Arguably we could use Owned or Options of pointers here, but in
its current form I would opt to leave the NULL check in to help the reader.
On Tue, Mar 29, 2016 at 4:39 PM, Klaus Ma <[email protected]> wrote:
> +1
>
> Refer to this doc for the detail of deleting null:
> http://www.cplusplus.com/reference/new/operator%20delete/ <
> http://www.cplusplus.com/reference/new/operator%20delete/>
>
> Thanks
> Klaus
>
> > On Mar 30, 2016, at 07:24, Neil Conway <[email protected]> wrote:
> >
> > On Tue, Mar 29, 2016 at 7:19 PM, <[email protected]> wrote:
> >> --- a/src/sched/sched.cpp
> >> +++ b/src/sched/sched.cpp
> >> @@ -1808,6 +1808,10 @@ MesosSchedulerDriver::~MesosSchedulerDriver()
> >> delete process;
> >> }
> >>
> >> + if (credential != NULL) {
> >> + delete credential;
> >> + }
> >
> > `delete` of a NULL pointer is safe, so I would vote for removing the
> `if`.
> >
> > Neil
>
>
