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

Moar unsignedness love #49

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Moar unsignedness love #49

wants to merge 1 commit into from

Conversation

alexsnaps
Copy link
Contributor

Kinda hacked in again…
All this would deserve tests, kinda pushing as I go :( Sorry

@alexsnaps alexsnaps closed this Aug 21, 2019
@alexsnaps alexsnaps reopened this Aug 21, 2019
@alexsnaps
Copy link
Contributor Author

btw, I think this may have not been the best approach… shouldn't a literal wrap a i128? And then let the "downstream" consumer see how it deals with it? wdyt?

@ms705
Copy link
Owner

ms705 commented Aug 21, 2019

Yeah, I think that's perhaps the best plan. Literal is never stored in large volumes, so wasting some memory on it is fine (unlike with DataType).

This kind of slightly yucky conversion code is likely best placed where we actually need to convert to sized types (e.g., in noria-mysql).

Another approach would be to represent types properly as enum variants that are part of Literal. This requires a larger change, but will result in a more standard parser datastructure; on the flip side, it will much complicate some code paths in Noria that handle literals.

@alexsnaps
Copy link
Contributor Author

That's a proposal for Literal::Integer(i128). Let me know if you'd rather have the other approach instead. If so tho, what granularity would you expect?

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