Skip to content

Conversation

@KaganCanSit
Copy link
Contributor

@KaganCanSit KaganCanSit commented Mar 22, 2025

Hello,

This draft PR includes updates to modernize the codebase by replacing deprecated mem_ops.h function calls with their std::span-based counterparts in line with C++20 standards. Additionally, I also discussed some TODO comments that I thought would be quick fixes.

My reference points the C++ Core Guidelines and CppReference. Since I am not actively developing in C++20 at the moment, I am marking this PR as a draft to review and feedback.

Changes made:

  • Replaced xor_buf calls with std::span-based equivalents.
  • Handled several smaller TODOs identified in the codebase.

Confirmed that builds and test suites pass with:

ninja clean && ./configure.py --without-documentation --cc=clang --compiler-cache=ccache --build-targets=static,cli,tests --build-tool=ninja && ninja && ./botan-test

Check code format with:

python3 src/scripts/dev_tools/run_clang_format.py --clang-format=clang-format-17 --src-dir=src

Outstanding considerations:
I intentionally did not modify the following function, as it touches multiple areas and could introduce bugs without extensive review:

// TODO: deprecate and replace, use .subspan()
inline void xor_buf(std::span<uint8_t> out, std::span<const uint8_t> in, size_t n) {
   BOTAN_ARG_CHECK(out.size() >= n, "output span is too small");
   BOTAN_ARG_CHECK(in.size() >= n, "input span is too small");
   xor_buf(out.first(n), in.first(n));
}

Files affected by this function;

src/lib/modes/aead/eax/eax.cpp
src/lib/mac/cmac/cmac.cpp
src/lib/pubkey/dlies/dlies.cpp

Additional notes:
There is a change in TLS, so additional checking may be required.

Thanks in advance for your time and guidance.

@KaganCanSit KaganCanSit marked this pull request as draft March 22, 2025 03:55
@KaganCanSit KaganCanSit force-pushed the feature-handling-some-todo-messages branch from e1d1cb2 to 47f27ae Compare March 22, 2025 10:27
@coveralls
Copy link

coveralls commented Mar 22, 2025

Coverage Status

coverage: 90.422% (+1.2%) from 89.189%
when pulling 1e9606e on KaganCanSit:feature-handling-some-todo-messages
into f2257be on randombit:master.

reneme

This comment was marked as resolved.

@KaganCanSit

This comment was marked as resolved.

@KaganCanSit KaganCanSit force-pushed the feature-handling-some-todo-messages branch 2 times, most recently from 7356fa8 to bfc033b Compare March 27, 2025 14:05
@KaganCanSit KaganCanSit marked this pull request as ready for review March 27, 2025 15:29
@KaganCanSit KaganCanSit requested a review from reneme March 27, 2025 15:29
Copy link
Collaborator

@reneme reneme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me now. Thank you for taking this on!

For the record: The mem_ops.h header is marked as "future internal header". So, technically, we don't even need to mark the typecast_copy and xor_buf overloads as "deprecated", because for library users the entire header is going to go away in a future major release.

Personally, I still think your change in mem_ops.h makes sense as-is, for documentation purposes.

@randombit, over to you. :)

@reneme reneme requested a review from randombit March 28, 2025 07:56
@KaganCanSit KaganCanSit force-pushed the feature-handling-some-todo-messages branch from bfc033b to 86c9a43 Compare March 28, 2025 11:05
@KaganCanSit KaganCanSit force-pushed the feature-handling-some-todo-messages branch from 86c9a43 to 837c30c Compare June 22, 2025 20:01
@KaganCanSit

This comment was marked as outdated.

@KaganCanSit KaganCanSit requested a review from reneme June 28, 2025 16:36
@KaganCanSit KaganCanSit force-pushed the feature-handling-some-todo-messages branch from b376fe2 to 5375c3c Compare July 10, 2025 18:47
@KaganCanSit

This comment was marked as outdated.

Copy link
Collaborator

@reneme reneme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience with this. I left a few suggestions for simplification (mostly unnecessary explicit construction of std::span).

Regarding the changes in xts.cpp: I think we should postpone those until some more top-down modernization has landed. Most notably #4880 and similar effort to spanify process_msg and start_msg. Once that is done, the abstract interface of all cipher mode implementations will be purely span-based and ready for such modernizations.

@KaganCanSit KaganCanSit force-pushed the feature-handling-some-todo-messages branch 3 times, most recently from 4b2c40a to 0698936 Compare July 13, 2025 12:57
@KaganCanSit
Copy link
Contributor Author

KaganCanSit commented Jul 13, 2025

I have addressed all feedback from the review:

roughtime std::span modernization proposal BufferSlicer is noted for future work - I may address this in a separate PR once it is merged. Comment

I will follow the results of the CI tests and then we can merge if you see fit. I see you're quite busy with clang-tidy today, I'll rebase it from time to time if needed, just FYI.

Copy link
Collaborator

@reneme reneme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks for your efforts.

@KaganCanSit
Copy link
Contributor Author

KaganCanSit commented Jul 14, 2025

Looks good to me. Thanks for your efforts.

Thank you for your time and for sharing your expertise. Your guidance has been very helpful throughout this process.

I’ll need to spend some additional time getting more familiar with the span construct. Since I'm currently use the C++11 standard, I've not using it in job.

@randombit, this development is ready to be merged at your discretion.

Best regards.

@KaganCanSit KaganCanSit force-pushed the feature-handling-some-todo-messages branch from 0698936 to a14398a Compare July 15, 2025 12:31
@KaganCanSit KaganCanSit force-pushed the feature-handling-some-todo-messages branch from a14398a to 89b2c15 Compare August 2, 2025 07:59
@KaganCanSit KaganCanSit force-pushed the feature-handling-some-todo-messages branch from 89b2c15 to c673d5c Compare August 16, 2025 17:33
@KaganCanSit KaganCanSit force-pushed the feature-handling-some-todo-messages branch 2 times, most recently from d60dafa to 237d1e6 Compare August 31, 2025 06:14
Copy link
Owner

@randombit randombit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the wait here. Changes are generally fine but the issue is mem_ops.h is currently a public header but I don't think the library should be in the business of providing general utility functions like this, which is why the header is deprecated and will be removed in Botan4. Any new functionality along these lines should go into the (internal) mem_utils.h header. Ideally we can update all of the callers within the library and then guard the declarations here, which are no longer used internally but which we must continue to provide for now for compatability, with !defined(BOTAN_IS_BEING_BUILT) as is done with various other functions, so it's not possible to add new code to the library which uses them.

@KaganCanSit
Copy link
Contributor Author

KaganCanSit commented Sep 2, 2025

Sorry for the wait here. Changes are generally fine but the issue is mem_ops.h is currently a public header but I don't think the library should be in the business of providing general utility functions like this, which is why the header is deprecated and will be removed in Botan4. Any new functionality along these lines should go into the (internal) mem_utils.h header. Ideally we can update all of the callers within the library and then guard the declarations here, which are no longer used internally but which we must continue to provide for now for compatability, with !defined(BOTAN_IS_BEING_BUILT) as is done with various other functions, so it's not possible to add new code to the library which uses them.

I recall we previously discussed removing the header file here. Actually, adding a new function for ‘mem_ops.h’ in this branch is not currently on the agenda. I was only focused on the TODO message; my goal was to deprecate the use of existing function calls and update them with std::span-based overloads.

Within the project, the use of functions marked with the declaration BOTAN_DEPRECATED(‘This function is deprecated, use the range-based version instead’) has now ended. These can be completely removed. However, since there may still be users who use them, I thought it would be prudent to declare them BOTAN_DEPRECATED. But additionally, to ensure they are not reused within the project, they can be wrapped with the !defined(BOTAN_IS_BEING_BUILT) macro, as you suggested.

I thought moving them to the (internal) mem_utils.h header would be a separate task. I'm not sure, maybe I misunderstood.

Some time has passed, so I need to review it again, but if you guide me step by step, I can work on it as soon as possible and apply the update you want.

@reneme
Copy link
Collaborator

reneme commented Sep 29, 2025

Given that this PR doesn't add any functionality to mem_ops.h, I would suggest the following steps :

  1. Let's change the deprecation messages in mem_ops.h from "This function is deprecated, use range-based version instead" to something along the lines of "This function is deprecated without a replacement". That wording doesn't suggest that we're planning to provide support for this function post-Botan4.
  2. Merge this PR (because the comment history is becoming unwieldy)
  3. In a follow-up PR: Move the declarations and internal usages of the relevant functions over to the internal mem_utils.h, guard mem_ops.h with !defined(BOTAN_IS_BEING_BUILT) for backward compatibility, and eventually remove it in Botan4

@KaganCanSit
Copy link
Contributor Author

KaganCanSit commented Sep 29, 2025

Given that this PR doesn't add any functionality to mem_ops.h, I would suggest the following steps :

  1. Let's change the deprecation messages in mem_ops.h from "This function is deprecated, use range-based version instead" to something along the lines of "This function is deprecated without a replacement". That wording doesn't suggest that we're planning to provide support for this function post-Botan4.
  2. Merge this PR (because the comment history is becoming unwieldy)
  3. In a follow-up PR: Move the declarations and internal usages of the relevant functions over to the internal mem_utils.h, guard mem_ops.h with !defined(BOTAN_IS_BEING_BUILT) for backward compatibility, and eventually remove it in Botan4

Thank you for informing me and guiding me @reneme. I would like to take on this task in this thread and in the pull request that will be opened later. Is @randombit suitable for you?


Updates:
To avoid further clutter in the comments, I will update this comment.
I am implementing the solution proposed by @reneme here. If deemed appropriate and these changes are merged, I am ready to take part in developing the remaining changes. For this, I will wait for this branch to be merged. (I don't want to repeatedly rebase a new branch based on this branch with every change.)

  • 09.11.25: Rebased to main branch.
  • 01.01.26: Rebased to main branch.

@KaganCanSit KaganCanSit force-pushed the feature-handling-some-todo-messages branch from 237d1e6 to cc0aec0 Compare October 3, 2025 19:19
@KaganCanSit KaganCanSit force-pushed the feature-handling-some-todo-messages branch from cc0aec0 to 19299fe Compare October 12, 2025 14:46
@KaganCanSit KaganCanSit force-pushed the feature-handling-some-todo-messages branch 2 times, most recently from 7fa74c1 to b1d6285 Compare November 9, 2025 06:38
@KaganCanSit KaganCanSit force-pushed the feature-handling-some-todo-messages branch from b1d6285 to 1e9606e Compare January 1, 2026 19:04
@KaganCanSit
Copy link
Contributor Author

KaganCanSit commented Jan 1, 2026

Hello @randombit, @reneme,

First, I wish you a healthy, peaceful, and successful New Year. I hope to contribute more to the project in 2026.

I wanted to share a note regarding this long-standing development branch. Some of my branches have been open for a long time and for some I’ll even need to revisit the details to fully recall what I did (there are ones approaching a year). For this reason, we could follow the approach @reneme outlined in this comment.

Another idea I have is: if avoiding changes to the header file (mem_ops.h) is important for you, then there aren’t actually any new functions added here. This PR only introduces deprecation annotations. If preferred, we could update the usages within the project to use std::span without making any changes to the header. That way, future modifications will be easier and will be future for Botan4 only change header file.

I would really like to contribute to this development. Unfortunately, I don’t have much practical experience with std::span yet, as I haven’t been using the latest standard extensively. However, I am always open to your guidance and feedback.

Kind regards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants