Update typedefs of C-style function pointers to std::function

[SeanCurtis-TRI]

This allows for the callback to maintain their own state (without recourse to globals), making it compatible with lambdas, bindings, functors, etc.)

In addition, added some incidental clean up:

• URICallback, passed by pointer, is now asserted to be non-null before accessing.
• FSCallbacks are validated when they are set (in debug builds).

[syoyo]

Does this change break the build(API breakage) for an existing app which uses C style function pointers?

Also, please do not use assert. I'm trying to remove assert to make the code secure&robust. #425

[SeanCurtis-TRI]

(1) It shouldn't break the API.

• Any place currently assigning a C-style function pointer will be accepted happily. They can all be implicitly converted to the comparable std::function.
  (2) Sorry about the assert, I saw it used somewhere else in the code to confirm a non-null object and assumed it was the preferred mechanism. What would you like instead? I'm not sure what practice you use to confirm expected prerequisites. For the case where the file system callbacks get passed, that would be a reasonable time to simply throw an exception. For the internals, we can just assume non-null like you do elsewhere.

[syoyo]

(1) It shouldn't break the API.

• Any place currently assigning a C-style function pointer will be accepted happily. They can all be implicitly converted to the comparable std::function.

Thanks!

(2) Sorry about the assert, I saw it used somewhere else in the code to confirm a non-null object and assumed it was the preferred mechanism. What would you like instead? I'm not sure what practice you use to confirm expected prerequisites. For the case where the file system callbacks get passed, that would be a reasonable time to simply throw an exception. For the internals, we can just assume non-null like you do elsewhere.

In the case of this PR, I think simply check if a callback(std::function) is non-empty/callable should be enough.
https://stackoverflow.com/questions/21806632/how-to-properly-check-if-stdfunction-is-empty-in-c11

Something like this:

if (cb) {
  cb(...)
} else {
  // optionally set error message.
  return false; 
}

[SeanCurtis-TRI]

The question is not how to determine if the callback is defined or not. The question is what to do about it. Sometimes you test, sometimes you don't. Sometimes when you test you do so in a context where you do have access to an error string and can return false to indicate failure. Other times, you can't.

Specifically, I'm thinking of the functions like SetURICallbacks() or SetFsCallbacks(). Both of those functions return void. In SetURICallbacks() you already had an assert on callbacks.decode being defined. In SetFsCallbacks() you didn't do any check at all. But in both of those, there's no error string and no return value. So, what would you like the code to do if the callbaks are incompletely specified?

One option is to do nothing and wait until we try to use the functions to report that they are undefined. The other is throw an exception at the time they are setting the callbacks (at least that won't be during parsing). We could also change the return value so it returns a pair like std::pair<bool, std::string>> where bool reports success and the std::string only contains a value if the bool is false. This kind of API change would be pretty safe as, currently, no one is catching any kind of return value. So, going from void to std::pair is relatively safe.

[syoyo]

Specifically, I'm thinking of the functions like SetURICallbacks() or SetFsCallbacks()
So, what would you like the code to do if the callbaks are incompletely specified?

Ah, I see. Ideally, such a method should have return type 'bool'(and return false when error), and also have the mechanism to report error message.

(SetURICallbacks was contributed by a contributor, and at that time I accepted assert for some reason. It'd also be better to remove this assert in this PR)

We could also change the return value so it returns a pair like std::pair<bool, std::string>>

I'd rather go with bool as return type and take a std::string(default to nullptr) to the last argument of the method:

bool SetURICallbacks(URLcallbacks, std::string *err = nullptr)

This also won't break API usage in existing app code.

[SeanCurtis-TRI]

Alrighty, I've removed all of the asserts (including the one from the previous PR) and updated the PRs as requested.

I've confirmed this builds for my own purposes, but I don't call all of the callback functions. You might want to run it through any further testing of your own.

[syoyo]

Thanks! Merged!

I've confirmed this builds for my own purposes, but I don't call all of the callback functions.

At least CI & unit test passes. I may add some unit tester later.

[SeanCurtis-TRI]

Out of curiosity, what is your policy for creating new releases? Or, to put it another way, if you had to guess how long it'll be before this latest commit ends up in a tagged commit, what would your guess be?

[syoyo]

Basically it follows with semvar rule.

• No API breakage, fixes and improvements : bump micro version
• Some API changes, new features or drastic changes: bump minor
• Quite large changes: bump major

[SeanCurtis-TRI]

And I notice you just released a new tag. :) Thanks.