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

Bounds checking in range() #1738

Closed

Conversation

jacqueswww
Copy link
Contributor

@jacqueswww jacqueswww commented Nov 23, 2019

What I did

  • Add some annotations.
  • Alters ast.py slightly to improve annotations.

How I did it

How to verify it

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@charles-cooper
Copy link
Member

this needs to be rebased off master (and the stray commit removed)

if not SizeLimits.in_bounds(typ, val):
raise InvalidLiteralException('Invalid range value supplied', self.stmt)

def _get_outtype(self, arg0_expr: LLLnode, arg1_expr: LLLnode) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

I think this code will be a lot clearer if this is called _get_range_type.

jacqueswww and others added 2 commits December 18, 2019 10:56
Co-Authored-By: Charles Cooper <cooper.charles.m@gmail.com>
@jacqueswww
Copy link
Contributor Author

jacqueswww commented Dec 18, 2019

@fubuloubu @charles-cooper made changes as suggested; I think it ready to be merged now?

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 18, 2019

This pull request introduces 1 alert when merging eb9e5e2 into 391b251 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

@@ -570,7 +570,8 @@ def parse_for(self):
if num_of_args == 1:
arg0_expr = self._get_range_const_value_expr(arg0)
arg0_val = arg0_expr.value
out_type_str = arg0_expr.typ.typ
out_type = arg0_expr.typ
start = LLLnode.from_list(0, typ=out_type, pos=getpos(self.stmt))
start = LLLnode.from_list(0, typ=out_type_str, pos=getpos(self.stmt))
Copy link
Member

Choose a reason for hiding this comment

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

This... doesn't seem right

@charles-cooper
Copy link
Member

Can we also rename out_* to range_*? Also I feel like the logic in parse_for could be pared down. It's a lot to follow right now.

@fubuloubu
Copy link
Member

Should be fixed by type checking work

@fubuloubu fubuloubu closed this Jun 22, 2020
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.

None yet

3 participants