Skip to content
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

Add helpers for modular arithmetic #634

Merged
merged 5 commits into from
Nov 13, 2024
Merged

Conversation

chiphogg
Copy link
Collaborator

The prime-testing techniques we will use (Miller-Rabin, Strong Lucas) all make heavy usage of modular arithmetic. Therefore, we lay those foundations here, adding utilities to perform the basic arithmetic operations robustly. Getting these foundations right is extremely important for the work that will follow.

Since these are internal-only helper functions, we don't bother checking the preconditions, although we state them clearly in the contract comment for each utility. After C++26, we could add contracts for these.

Helps #509.

The prime-testing techniques we will use (Miller-Rabin, Strong Lucas)
all make heavy usage of modular arithmetic.  Therefore, we lay those
foundations here, adding utilities to perform the basic arithmetic
operations robustly.

Since these are internal-only helper functions, we don't bother checking
the preconditions, although we state them clearly in the contract
comment for each utility.  After C++26, we could add contracts for
these.

Helps mpusz#509.
Apparently some freestanding builds need this?
@@ -32,6 +32,7 @@
import std;
#else
#include <array>
#include <bits/types.h>
Copy link

@CJCombrink CJCombrink Nov 11, 2024

Choose a reason for hiding this comment

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

This is normally not the correct header. A tool like clangd will suggest this include due to the internals of the clangd compiler, which matches in most cases the gcc compiler.
For most compilers a higher level include is normally need, as this one is an implementation detail of the specific compiler.
Unfortunately I am not sure what the correct C++ header is that must be included

Choose a reason for hiding this comment

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

I quickly looked at the compile error, it appears that <cstdint> is the correct high level include since it complains that 'uint64_t' must be declared before it is used and that should be found in the mentioned header according to https://en.cppreference.com/w/cpp/types/integer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! <cstdint> is already included, so I guess the error is just spurious.

Apparently, that's not the right approach.
Comment on lines +47 to +49
// Precondition: (a < n).
// Precondition: (b < n).
// Precondition: (n > 0).
Copy link
Owner

Choose a reason for hiding this comment

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

You can use MP_UNITS_EXPECTS_DEBUG if needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd love to express these in code, but here is what I see:

image

This is with #include <mp-units/compat_macros.h>.

Any advice for a quick fix?

Copy link
Owner

Choose a reason for hiding this comment

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

Sure 😉

#ifndef MP_UNITS_IN_MODULE_INTERFACE
#include <mp-units/ext/contracts.h>

// Precondition: (a < n).
// Precondition: (b < n).
// Precondition: (n > 0).
[[nodiscard]] consteval uint64_t add_mod(uint64_t a, uint64_t b, uint64_t n)
Copy link
Owner

@mpusz mpusz Nov 11, 2024

Choose a reason for hiding this comment

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

Usage of cstdint without the std:: namespace prefix will probably fail to compile in import std; build.

@chiphogg chiphogg requested a review from mpusz November 13, 2024 16:36
@mpusz mpusz merged commit 5cd07bc into mpusz:master Nov 13, 2024
74 of 76 checks passed
@chiphogg chiphogg deleted the chiphogg/mod#509 branch November 13, 2024 19:56
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.

3 participants