Skip to content

Conversation

@tbrezot
Copy link
Contributor

@tbrezot tbrezot commented Apr 25, 2025

An arguably simpler version of the MemoryADT:

  • rely on the semantics of IntoIterator for an option to merge the two branches of the if in guarded_write
  • remove the fold+unzip and recreate result in linear time in batch_read

@tbrezot tbrezot requested review from HatemMn and Manuthor April 25, 2025 10:26
Copy link
Contributor

@HatemMn HatemMn 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 ! the few comments are just form, I think this will stop the stack overflowing


pub(crate) fn findex_number_of_threads() -> Option<usize> {
std::env::var("GITHUB_ACTIONS").is_ok().then_some(1)
std::env::var("GITHUB_ACTIONS").map(|_| 1).ok()
Copy link
Contributor

Choose a reason for hiding this comment

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

while 100% equivalent the old syntax was more verbose and hence easier to understand for me but feel free to close this if you prefer the new one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a concurrent modification I kept through the rebase. I guess which version is more readable depends on personal taste. I for example feels the other version to be less straightforward since it involves more Rust-specific helper functions (I don't know by heart the signature of is_ok(), while map is standard).

I can revert this change if you prefer.

@tbrezot
Copy link
Contributor Author

tbrezot commented Apr 26, 2025

Looks good ! the few comments are just form, I think this will stop the stack overflowing

This is a pure refacto and I do not expect it to solve the stack overflow issue at all.

@HatemMn
Copy link
Contributor

HatemMn commented Apr 28, 2025

all good, we can merge

@Manuthor Manuthor merged commit 95cee3f into develop Apr 28, 2025
15 checks passed
@Manuthor Manuthor deleted the refacto/kms_encryption_layer branch April 28, 2025 10:33
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