randombit / botan

Cryptography Toolkit
https://botan.randombit.net
BSD 2-Clause "Simplified" License
2.6k stars 570 forks source link

Fix: Deprecated BigInt::Base Enum Warnings #4303

Closed FAlbertDev closed 3 months ago

FAlbertDev commented 3 months ago

The deprecation of BigInt::Base is problematic when including bigint.h in applications. Since the BigInt::Base is deprecated, we cannot declare methods like:

BOTAN_DEPRECATED("For hex/decimal use from_string") BigInt(const uint8_t buf[], size_t length, Base base);

This produces errors in applications like (at least with MSVC):

build/include/public\botan/bigint.h(132): warning C4996: 'Botan::BigInt::Base': All functions using this enum are deprecated
build/include/public\botan/bigint.h(888): warning C4996: 'Botan::BigInt::Base': All functions using this enum are deprecated
build/include/public\botan/bigint.h(896): warning C4996: 'Botan::BigInt::Base': All functions using this enum are deprecated

Instead, I propose only deprecating the enum values.

I'm working on a regression test by writing an example using the amalgamation header in amalgamation ci jobs. Similar issues should be discovered this way.

reneme commented 3 months ago

I'm working on a regression test [...]. Similar issues should be discovered this way.

Let me add that this type of warning is easily missed by our current CI. As far as I know, we don't really "consume" our headers from a user perspective: namely, without -DBOTAN_IS_BEING_BUILT. This hides such (likely compiler-dependent) issues.

coveralls commented 3 months ago

Coverage Status

coverage: 91.28% (-0.004%) from 91.284% when pulling f5c629db8a2bdc68d978a76901c5a5034aca9808 on Rohde-Schwarz:fix/deprecated_enum into 5f4c2c3e427fde525377e23cdaeee845d534e757 on randombit:master.

randombit commented 3 months ago

Let me add that this type of warning is easily missed by our current CI. As far as I know, we don't really "consume" our headers from a user perspective: namely, without -DBOTAN_IS_BEING_BUILT. This hides such (likely compiler-dependent) issues.

This is so. This even managed to let us ship a header that Clang rejected as invalid (#4234)

reneme commented 3 months ago

Leveraging our time zone difference here and fishing for early feedback: 😏

@FAlbertDev's idea is to extend the 'amalgamation' CI target (as it practically includes all headers at once) and create a super simple example file that showcases the include of botan_all.h. The make examples target does not set -DBOTAN_IS_BEING_BUILT, already now. Perhaps as the only build target in the project.

Hence, compiling the examples in the amalgamation build with -Werror could be a reasonable way to catch such things in the future, on all three major compilers, with fairly minimal effort.