Closed SeanCurtis-TRI closed 3 weeks ago
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. https://github.com/syoyo/tinygltf/issues/425
(1) It shouldn't break the API.
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.(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;
}
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.
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.
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.
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.
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?
Basically it follows with semvar rule.
And I notice you just released a new tag. :) Thanks.
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: