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

aya/maps: allow constructing HashMaps from LruHashMap #774

Merged
merged 1 commit into from
Sep 1, 2023

Conversation

ajwerner
Copy link
Member

@ajwerner ajwerner commented Aug 31, 2023

The macro to implement TryFrom for MapData didn't have the ability to
specify that more than one variant of MapData can be valid for a single
map implementation. Support for new syntax was added to the macro so that
the implementation can succeed for both valid variants in the HashMap
and PerCpuHashMap impls.

Fixes #636


This change is Reviewable

@netlify
Copy link

netlify bot commented Aug 31, 2023

Deploy Preview for aya-rs-docs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 172859c
🔍 Latest deploy log https://app.netlify.com/sites/aya-rs-docs/deploys/64f20106568f050008ad7b7a
😎 Deploy Preview https://deploy-preview-774--aya-rs-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mergify mergify bot added the aya This is about aya (userspace) label Aug 31, 2023
@ajwerner ajwerner marked this pull request as ready for review August 31, 2023 12:24
@ajwerner ajwerner requested a review from tamird August 31, 2023 15:01
impl_try_from_map_generic_key_and_value!(HashMap, PerCpuHashMap, LpmTrie);
impl_try_from_map_generic_key_and_value!(
(HashMap|LruHashMap) => HashMap,
PerCpuHashMap,
Copy link
Member

Choose a reason for hiding this comment

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

Does PerCpuLruHashMap have the same bug?

@mergify
Copy link

mergify bot commented Sep 1, 2023

@ajwerner, this pull request is now in conflict and requires a rebase.

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ajwerner)


aya/src/maps/hash_map/per_cpu_hash_map.rs line 152 at r2 (raw file):

#[cfg(test)]
mod tests {

nit: this empty line is inconsistent with other modules

The macro to implement TryFrom for MapData didn't have the ability to
specify that more than one variant of MapData can be valid for a single
map implementation. Support for new syntax was added to the macro so that
the implementation can succeed for both valid variants in the HashMap
and PerCpuHashMap impl.

Fixes aya-rs#636
Copy link
Member Author

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

TFTR!

Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @ajwerner)


aya/src/maps/hash_map/per_cpu_hash_map.rs line 152 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

nit: this empty line is inconsistent with other modules

Done.

Copy link
Member

@tamird tamird left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ajwerner)

@ajwerner ajwerner merged commit 8d3fc49 into aya-rs:main Sep 1, 2023
20 checks passed
@ajwerner ajwerner deleted the try_from_LruHash branch September 1, 2023 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aya This is about aya (userspace)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cant' use LruHashMap in userspace
2 participants