[move-compiler] Intern WarningFilters
#20141
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
tnowacki
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
| /// An intern table for warning filters. The `Pin` is likely unnecessary given the usage of | ||
| /// `WarningFiltersTable`, but it is added to ensure the underlying `Box` is not moved. | ||
| /// Safety: This table should not be dropped as long as any `WarningFilters` are alive | ||
| pub struct WarningFiltersTable(HashSet<Pin<Box<WarningFiltersBuilder>>>); |
There was a problem hiding this comment.
Open to suggestions about my intern table here. Maybe there is something slightly more clever we can do?
There was a problem hiding this comment.
I have a pretty drastic change suggestion:
What if we number the well-known filters? And then we can use a u64 (or, if we ever get more than 64, go up in size to a u128/u256/bitset). All can be u64::MAX, none is 0, and we just do mask checks. Then interning is just writing down that specific u64, and checking is a quick walk down a chain of those.
There was a problem hiding this comment.
Also I agree with the comment, I don't think you need Pin here. Try it without and see if it breaks?
There was a problem hiding this comment.
I removed the pin and all seems to be well.
I looked into the bitset approach, but I got stuck with external filters. The warning filters being allowed to come from outside sources for warnings not defined by the compiler makes this all a bit tricky
| @@ -79,6 +82,7 @@ pub(super) struct DefnContext<'env, 'map> { | |||
| struct Context<'env, 'map> { | |||
| defn_context: DefnContext<'env, 'map>, | |||
| address: Option<Address>, | |||
| warning_filters_table: Mutex<WarningFiltersTable>, | |||
There was a problem hiding this comment.
I kept the Arc out of the WarningFiltersTable definition to allow for things like this. During construction, the table doesn't need the Arc, just this single Mutex. But once it is done, it needs to be in the Arc to prevent deep clones--which would be unsafe
tnowacki
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
tnowacki
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
tnowacki
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
Description
Originally, I had just thrown an
ArcaroundWarningFilters... but it felt a bit silly since I never really needed to modify them. I wanted to do something that leveraged their immutability. So I went a bit overboard with this somewhat shoddy intern table. Seems to work fine though :)Test plan
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.