Don't crash if NULL is passed to mrb_close

[tatsuhiro-t]

Sometimes it is very useful just return from mrb_close if NULL is
passed as mrb. This is the same spirit of free(3), which just does
nothing if NULL is passed.

[franckverrot]

Should we change the semantics of mrb_close in order to return meaningful details about what happened there? Or would it be at a minimum OK to have it warn on stderr that NULL was passed?

[cremno]

This is the same spirit of free(3), which just does nothing if NULL is passed.

I'm not opposing merging this but note that isn't true for fclose() or lua_close() since NULL isn't a valid pointer to FILE or lua_State.

@franckverrot: Why?

[franckverrot]

@cremno Thinking in terms of side-effects, free either fails, frees memory or doesn't do anything, and as an observer (from the call-site) there's no way to determine what happened (unless you checked for NULL at some point, but calling free could have been avoided in that case).

I think it's a good thing to avoid crashes when we call mrb_close(NULL), but I personally don't like the idea of mixing the “closes the context” and ”does nothing” side-effects.

[matz]

do you have any use-case in mind? mrb_close(NULL) is a bug, isn't it?

[tatsuhiro-t]

My usecase is that when mrb_state is a data field of C++ struct and it could be NULL, then we don't have check whether it is NULL in destructor:

struct Foo {
  Foo(...) : state(nullptr) {
    if (some_condition_met()) {
      state = mrb_open();
    }
  }

  ~Foo() {
    mrb_close(mrb);
  }

  mrb_state *state;
};

As commented earlier, many library functions, like fclose or SSL_free from OpenSSL, will crash if NULL is passed. In those functions, we have to check the argument is NULL or not. Many programs will do this for safety. If it is done in mrb_close, we can save those lines of code.
I always allow passing NULL to these "free" functions when I design this kind of function.

[murasesyuka]

👍 I think RAII is good Idiom. and want to Error output.

it warn on stderr that NULL was passed?

@tatsuhiro-t typo? s/mrb_close(mrb);/mrb_close(state)/g

[matz]

OK, adding 1 line would not harm.

[franckverrot]

@matz Isn't a good case to use mrb_assert instead?

If not, would you be OK to change the semantics of mrb_close to return:

1. a success value if the whole function ran;
2. a warning/error value if we returned early due to the precondition of having mrb != NULL not fulfilled?

It'd be the best of both worlds: don't check what you pass to mrb_close if you don't want to, but still provide a way to know what happened there with the value it returns.

[cremno]

Either mrb_close() accepts a null pointer as argument and immediately returns (this merged PR) or it doesn't (before the merge but mrb_assert() would've been an improvement!).

Everything else would just be complicated. Simply check before the call if that info is needed for some reason.

[mattn]

I'm thinking mrb_close should not be calm for NULL. As you said, free(NULL) is, but fclose(NULL) is not.