Closed lieser closed 1 year ago
I general fine with this approach. For Serialized_RNG
we might still want a (deprecated) constructor that takes a raw pointer in addition to one consuming a unique_ptr
, but the others you mention are relatively obscure and I'd be fine with just using only unique_ptr
for those.
A good goal to shoot for IMO is to remove all uses of explicit new
(now that we have make_unique
available) in the same way that (almost) all uses of explicit delete
were removed a few years back in preference to RAII.
Definitely a good target for 3.x. Thanks for all the effort @randombit!
Thanks for the quick work on this. And nice to see that you took the additional effort on also getting rid of new/delete in Botan's internal code.
I had a quick look what is still missing, and found the following APIs there currently raw pointers with transferred ownership is used, and now newer API seems to be available:
Cipher_Mode_Filter
PBKDF2_Family
ctorPKCS5_PBKDF2
ctorOpenPGP_S2K
ctorRFC4880_S2K
ctorRFC4880_S2K_Family
ctorX509::load_key()
X509::copy_key()
DLIES_Encryptor
ctorDLIES_Decryptor
ctorDLIES_Decryptor
ctorPKCS10_Request::subject_public_key()
X509_Certificate
has std::unique_ptr<Public_Key> load_subject_public_key()
as an alternative, but this is missing here@randombit
I think the constructors are not that important, as a new overload can be easily added. But if for X509::load_key()
and X509::copy_key()
the return value should be changed instead of adding new alternative functions this needs to happen before the final 3.0.0.
We should probably remove:
PublicKey* X509_Certificate::subject_public_key()
... it returns a raw pointer and documentation claims that the caller is responsible for the object. New code shall use load_subject_public_key()
instead. Probably, the old method can go with 3.0.
X509_Certificate::subject_public_key()
I believe all of the above mentioned APIs have now been updated to using smart pointers, with the exception of the filters code. For the filters, I am inclined to just leave things exactly as they are. Already the documentation states that no further development is being done there, and recommends avoiding it in new code.
I think it would be good if Botan 3.x would start using
std::unique_ptr
in its API there ownership is transferred, instead of raw pointers. In some cases it should also be possible to just add it as an overload, if you prefer to still allow passing a raw pointer too.I did not look at the complete code there raw pointer are currently used to transfer ownership. Below are only the instances there we currently need to disable the using new warnings in the static analysis of our code:
Serialized_RNG
ctorCertificate_Extension::copy()
Extensions::add(...)
(add_new
/replace
too)/cc @securitykernel