-
Notifications
You must be signed in to change notification settings - Fork 8
fix: initialise default tsa_uri in c2pa::Signer constructor with an empty string
#104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: initialise default tsa_uri in c2pa::Signer constructor with an empty string
#104
Conversation
Constructing a string from a nullptr is an undefined behavior and is disallowed in c++23
|
Please note that the ADOBE CLA was signed from Also, request add suitable reviewer to the PR and please provide review comments and approve. Thanks for all help. If any questions: Please contact [email protected] |
|
@gpeacock : Please add suitable reviewers! |
| Signer(C2paSigner *signer) : signer(signer) {} | ||
|
|
||
| Signer(const string &alg, const string &sign_cert, const string&private_key, const string &tsa_uri = NULL); | ||
| Signer(const string &alg, const string &sign_cert, const string&private_key, const string &tsa_uri = {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to change this to the following, if we are going to make a change to the API:
Header
Signer::Signer(const string &alg, const string &sign_cert, const string &private_key, optional<std::string> tsa_uri)Implementation
// implementation then would be something like this:
Signer::Signer(const string &alg, const string &sign_cert, const string &private_key,
optional<std::string> tsa_uri) {
auto info = C2paSignerInfo{
alg.c_str(),
sign_cert.c_str(),
private_key.c_str(),
tsa_uri.has_value() ? tsa_uri->c_str() : nullptr
};
signer = c2pa_signer_from_info(&info);
}Since we are already changing the API, let's make it more semantically correct. Also, using a string as the default changes the behavior of the API. Using a zero length string would mean we are passing a non-nullptr to the Rust code, which may change the behavior of the rust sdk. Lemme know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already include in the headers and use it in other apis, so this is the right approach here.
FYI, we are a little inconsistent, usually just referring to optional instead of std:optional in the source.
Constructing a
std::stringfrom anullptris undefined behavior and has been removed in C++23.This fix ensures well-defined behavior and allows the project to build successfully when using the C++23 standard.