[std::span] BigInt - use std::span<> in some internal implementations

[reneme]

Pull Request Dependencies

• FIX: Code Indentation in bigint.h #3865
• [std::span] BigInt Constructors #3866

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 of clang-format.

This introduces std::span<> overloads for BigInt (mostly in the dependended upon PRs) and moves the high-level implementations to be based on std::span<>. It doesn't touch the implementations in big_code.cpp, big_rand.cpp, or big_ops*.cpp. We could do that as a follow-up, though.

[reneme]

@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().

[randombit]

I’ll look into what’s going with this

[coveralls]

coverage: 92.07% (+0.001%) from 92.069%
when pulling e707fdb on Rohde-Schwarz:span/bigint
into 042af03 on randombit:master.

[randombit]

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 mem_ops.h changes - span interfaces are certainly preferable BUT do we want to have those interfaces at all? That is should we spanify them, or should we instead deprecate and work to remove them entirely?

I really wish there was some way of getting behavior like pub(crate) acts in Rust where an interface is available freely within the library itself but completely unavailable for consumers. I guess we could do things like

#if defined(BOTAN_IS_BEING_BUILT)
void some_function_we_need_but_dont_want_to_expose_to_users();
#endif

but this may violate ODR, IDK.

For now can you split this into 3 PRs

• The reformatting (instant approve+merge)
• The constructor changes (same)
• The binary_decode etc changes (we'll have to figure out what we want to do)

[reneme]

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.

I really wish there was some way of getting behavior like pub(crate) acts in Rust

Mhm, that'd actually be nice. Perhaps in 5-10 years C++ modules will have evolved enough to model these semantics. 😞
Until then, we're probably stuck with things like those quick fixes:

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 users

class BigInt {
  public:
    void _leading_underscore_means_library_internal();
};

Polymorphy in detail namespace to expose additional APIs

class 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

[randombit]

Should we deprecate some of these ptr+len interfaces?

[reneme]

Yes please! :)

[randombit]

Going with an _ prefix to indicate to users that it’s public for (reasons) but not intended to application use sounds pretty good.