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

Fixes to Table model class #199

Merged
merged 1 commit into from
Jan 26, 2024
Merged

Fixes to Table model class #199

merged 1 commit into from
Jan 26, 2024

Conversation

dmlloyd
Copy link
Collaborator

@dmlloyd dmlloyd commented Jan 26, 2024

Make the limits immutable. Use the maximum limit constant 0x1_0000_0000 when the upper limit is unspecified, and simplify comparisons accordingly. There is a class which represents limits; change Table to reuse it.

Make the limits immutable. Use the maximum limit constant `0x1_0000_0000` when the upper limit is unspecified, and simplify comparisons accordingly.
There is a class which represents limits; change `Table` to reuse it.
@@ -1,19 +1,31 @@
package com.dylibso.chicory.wasm.types;

public class Limits {
private final int min;
private final int max;
public static final long LIMIT_MAX = 0x1_0000_0000L;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was wondering where this number comes from and I found this thread. Do you have a better link?
According to the follow up PR they are defining different limits for different things.
Should we specialize Limits for each usage?

Interested in hearing your reasoning around this 🙏

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Limits are used only where things are indexed by a 32-bit unsigned integer. Therefore "unlimited" is effectively the largest 32-bit unsigned integer plus one (because limits apply to sizes, not to indices).

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 think specializing limits might be a good idea, but I don't know if specializing it by type is necessarily the right thing, if we think of "limits" as a data structure.

We could possibly pursue that in a follow-up PR.

Copy link
Collaborator

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

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

Ok, given the explanation I think it's a nice cleanup, I would love another couple of eyes on this though.

@andreaTP andreaTP merged commit e1538d1 into dylibso:main Jan 26, 2024
6 checks passed
@dmlloyd dmlloyd deleted the tables branch January 26, 2024 18:10
danielperano pushed a commit that referenced this pull request Feb 1, 2024
Make the limits immutable. Use the maximum limit constant `0x1_0000_0000` when the upper limit is unspecified, and simplify comparisons accordingly.
There is a class which represents limits; change `Table` to reuse it.
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.

2 participants