mentioned in: bitcoin#33867
"> The way you had it written before is confusing because if the ptr is null, it is a logical conclusion that data_dir_len should also be zero.
should being the keyword here indeed. A non-zero length with a nullptr indicates the user clearly made an error, regardless of business logic. We need to return early in order to not cause any UB later on, and because the request is nonsensical. In the earlier approach on which you commented (0ae6a22a9f44b7e609431b9bac8bcf1b41d9a977), nullptr (but only with zero length) was allowed because it was not causing any errors on my machine, defaulting to the cwd. However, this was later proven a bad idea, hence the change in approach to now (6657bcb) disallow null and empty input in general.
For context, in bitcoin@5b89956 (to which this PR is a follow-up) we're distinguishing between "this call just doesn't make any sense, what are you doing?" and "unfortunately we can't produce a result with null input" - even though they both lead to a nullptr result. The distinction is just to let the appropriate code deal with each situation, even if it could be simplified.
I don't think it is required, and you could also simply say data_dir == nullptr || blocks_dir == nullptr?
In the current approach (6657bcb), we also want to disallow empty strings (""), so just checking for nullptr wouldn't suffice, we need the length check too for that.
I''m commenting here to further my understanding and to verify the logical condition, not because I consider myself an expert on Rust/C bindings in any sense.
I appreciate you taking the time to review this PR." - stickies-v
mentioned in: bitcoin#33867
"> The way you had it written before is confusing because if the ptr is null, it is a logical conclusion that data_dir_len should also be zero.
should being the keyword here indeed. A non-zero length with a nullptr indicates the user clearly made an error, regardless of business logic. We need to return early in order to not cause any UB later on, and because the request is nonsensical. In the earlier approach on which you commented (0ae6a22a9f44b7e609431b9bac8bcf1b41d9a977), nullptr (but only with zero length) was allowed because it was not causing any errors on my machine, defaulting to the cwd. However, this was later proven a bad idea, hence the change in approach to now (6657bcb) disallow null and empty input in general.
For context, in bitcoin@5b89956 (to which this PR is a follow-up) we're distinguishing between "this call just doesn't make any sense, what are you doing?" and "unfortunately we can't produce a result with null input" - even though they both lead to a nullptr result. The distinction is just to let the appropriate code deal with each situation, even if it could be simplified.
In the current approach (6657bcb), we also want to disallow empty strings (
""), so just checking for nullptr wouldn't suffice, we need the length check too for that.I appreciate you taking the time to review this PR." - stickies-v