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

Weak tables #15

Open
lenianiva opened this issue Sep 20, 2023 · 9 comments · Fixed by #17 · May be fixed by #20
Open

Weak tables #15

lenianiva opened this issue Sep 20, 2023 · 9 comments · Fixed by #17 · May be fixed by #20

Comments

@lenianiva
Copy link

Is there a way to use WHConsed as keys in a weak hash table like weak_table? If not I can write one and open a PR

@AdrienChampion
Copy link
Owner

Once you have a HConsed value, you can weak-ify it using to_weak.

@AdrienChampion
Copy link
Owner

I took a brief look at weak_table, and it seems to be compatible with them we need to implement some trait, probably WeakElement, and/or give access to Weak ref stored in WHConsed.

@AdrienChampion
Copy link
Owner

I prefer the latter as it avoids conditionally depending on yet another library, what do you think @vleni ?

@lenianiva
Copy link
Author

I took a brief look at weak_table, and it seems to be compatible with them we need to implement some trait, probably WeakElement, and/or give access to Weak ref stored in WHConsed.

I think giving access to the weak pointer inside would be a good idea. The to_weak function in HConsed doesn't give a Weak pointer though, and that is needed for weak_table.

@asvarga
Copy link

asvarga commented Feb 21, 2024

How is this intended to be used? I tried combining the hashconsing and weak_table doc demos, and the only thing I could get to work is:

let mut table = <WeakKeyHashMap<Weak<ActualTerm>, u32>>::new();
let one = var(1);
table.insert(one.to_weak_ref().upgrade().unwrap(), 1);    // yuck
assert_eq!(table.get(&one), Some(&1));

but this seems wrong/redundant.

@AdrienChampion
Copy link
Owner

AdrienChampion commented Feb 26, 2024

Hi @asvarga!

Sorry for the delay. I'm not sure how this is intended to be used as I don't use weak_table myself, it was a feature request.

However, looking at weak_table's API (for the first time), it seems like to_weak_ref is not really what one needs as insert takes a strong version of the reference. As we prefer not adding dependencies if we can avoid it, even optional ones, then the solution would be to have a HConsed::to_ref yielding the underlying Arc which would then work thanks to this implementation in the weak_table crate.

Then you could just write

let mut table = <WeakKeyHashMap<Weak<ActualTerm>, u32>>::new();
let one = var(1);
table.insert(one.to_ref(), 1);    // better (?)
assert_eq!(table.get(&one), Some(&1));

Am I making sense, am I missing something? Do you think that would be fine?

@AdrienChampion
Copy link
Owner

@asvarga @vleni I created #19 to get a sense of the interest in actual weak_table integration, which would be much nicer for you guys

@asvarga
Copy link

asvarga commented Feb 26, 2024

Thanks for your response, I kept wanting to do

table.insert(one.elm, 1);

but elm is private so HConsed::to_ref would work. I'll comment on #19 as well.

@AdrienChampion
Copy link
Owner

Sounds good, I'll merge that ASAP, thank you for your feedback 😺

@AdrienChampion AdrienChampion linked a pull request Feb 27, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants