(3) is not true, though: as written, there may or may not be a latch
(the code works correctly correct either way). Using an operation
(delete) that works fine for null pointers as a way to imply that a
pointer is NOT null does not seem like the best arrangement.
As you say, there should be better ways to communicate to the reader
that `process` and `credential` are optional but `latch` is not. For
example:
* encoding this into the type system using Owned / Option / etc.
* CHECKs (e.g., CHECK that `latch` is not NULL).
* comments
Neil
On Wed, Mar 30, 2016 at 3:24 PM, Benjamin Mahler <bmah...@apache.org> wrote:
> 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 <klaus1982...@hotmail.com> 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 <neil.con...@gmail.com> wrote:
>> >
>> > On Tue, Mar 29, 2016 at 7:19 PM, <vinodk...@apache.org> 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
>>
>>
