-
Notifications
You must be signed in to change notification settings - Fork 544
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
[std::span] BigInt - use std::span<> in some internal implementations #3855
base: master
Are you sure you want to change the base?
Conversation
void binary_decode(const uint8_t buf[], size_t length); | ||
* Store BigInt-value in a given byte array. If the buffer length | ||
* is less than the size of the value, then it will be truncated. | ||
* If it is greater than the size of the value, it will be zero-padded. |
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.
@randombit The implementation didn't seem to do actually ensure a zero-padding. I would have expected a call to clear_mem()
at least. I think that needs to be addressed somehow, but I'm not sure about the implications.
Similarly, in BigInt::encode_words()
.
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’ll look into what’s going with this
So the constructor changes (and of course the indentation) look fine. However for the rest, I think we're in a similar situation as with the I really wish there was some way of getting behavior like
but this may violate ODR, IDK. For now can you split this into 3 PRs
|
I opened independent PRs (#3865, #3866) for the simple changes as suggested. This PR is untouched and I'll rebase accordingly, once the new ones are merged.
Mhm, that'd actually be nice. Perhaps in 5-10 years C++ modules will have evolved enough to model these semantics. 😞 Change class member visibility for consumers#if defined(BOTAN_IS_BEING_BUILT)
#define PRIVATE_TO_BOTAN public
#else
#define PRIVATE_TO_BOTAN private
#endif
class BigInt {
PRIVATE_TO_BOTAN:
void library_internal_dont_touch();
public:
void to_string() const;
}; ... for this we should make sure that visibility is not mangled into the library symbol names. If that isn't the case, I don't think it would cause ODR issues. Underscores are a taboo for downstream usersclass BigInt {
public:
void _leading_underscore_means_library_internal();
}; Polymorphy in detail namespace to expose additional APIsclass BigInt {
protected:
void library_internal_dont_touch();
};
// could be in an internal header
namespace detail {
class BigInt : public Botan::BigInt {
public:
using library_internal_dont_touch;
};
} // namespace detail |
*/ | ||
void encode_words(word out[], size_t size) const; | ||
void encode_words(word out[], size_t size) const { encode_words(std::span{out, size}); } |
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.
Should we deprecate some of these ptr+len interfaces?
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.
Yes please! :)
Going with an |
Pull Request Dependencies
Description
Note: I split that into multiple commits for easier reviewability. Especially the first commit is fixing the indentation of
BigInt
's doxygen comments. I'm guessing that this is an artifact of the introduction ofclang-format
.This introduces
std::span<>
overloads forBigInt
(mostly in the dependended upon PRs) and moves the high-level implementations to be based onstd::span<>
. It doesn't touch the implementations inbig_code.cpp
,big_rand.cpp
, orbig_ops*.cpp
. We could do that as a follow-up, though.