-
-
Notifications
You must be signed in to change notification settings - Fork 34k
gh-143504: Expose CELL status of a symbol in symtable #143549
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: AN Long <aisk@users.noreply.github.com>
|
I'll fix this in a while, my apologies for quite a big fault in the code actually |
Lib/test/test_symtable.py
Outdated
| outer = find_block(top, "outer") | ||
| self.assertIn("x",outer.get_cells()) | ||
| self.assertTrue(outer.lookup("x").is_cell()) | ||
| self.assertFalse(outer.lookup("inner").is_cell()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put this test next to test_free.
Lib/symtable.py
Outdated
|
|
||
| def get_cells(self): | ||
| """Return a list of cell variable names in the table.""" | ||
| return [s.get_name() for s in self.get_symbols() if s.is_cell()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably belongs next to get_frees.
|
@iritkatriel I was forced to force push the symtable branch due to me updating the branch before for the CI. |
Lib/symtable.py
Outdated
|
|
||
| def get_cells(self): | ||
| """Return a list of cell variable names in the table.""" | ||
| return [s.get_name() for s in self.get_symbols() if s.is_cell()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this not follow the pattern of get_frees and get_nonlocals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll have to use the _cells variable then, I'll add it.
Lib/test/test_symtable.py
Outdated
| code="""def outer(): | ||
| x=1 | ||
| def inner(): | ||
| return x""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the other tests don't do all this. Can we follow the same pattern in this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iritkatriel I've implemented the change in symtable.py with get_cells
But the current test_cells seems to be the only one that keeps working after I've change it up quite a few times.
Is it really a requirement for it all to have the same pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there's a reason why we can't, then we should follow the same pattern. What's the difference between the cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.internal has x as FREE var (for test_free). we need x as CELL var (outer scope). setUp doesn't have that so inline code is needed. However I've removed the unnecessary comments and changed the rest of it to
top=symtable.symtable(code,"?","exec")
outer = find_block(top, "outer")
self.assertEqual(outer.get_cells(), ["x"])
self.assertTrue(outer.lookup("x").is_cell())
self.assertFalse(outer.lookup("inner").is_cell()) to use assert.Equal,true and false like the rest of the functions.
WOuld this be satisfactory as a commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.spam.lookup("x").is_cell()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and get_cells test should be next to get_frees test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and get_cells test should be next to get_frees test.
isn't test_cells already next to test_free?
| self.assertFalse(outer.lookup("inner").is_cell()) | ||
| self.assertTrue(self.spam.lookup("x").is_cell()) | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_cells is not tested at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add it to test_function_info:
@@ -255,6 +255,7 @@ def test_function_info(self):
self.assertEqual(sorted(func.get_locals()), expected)
self.assertEqual(sorted(func.get_globals()), ["bar", "glob", "some_assigned_global_var"])
self.assertEqual(self.internal.get_frees(), ("x",))
+ self.assertEqual(self.spam.get_cells(), ("some_var", "x",))
| """Return a list of cell variable names in the table. | ||
| """ | ||
| if self.__cells is None: | ||
| self.__cells = [s.get_name() for s in self.get_symbols() if s.is_cell()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's follow the pattern that it used in get_frees:
| self.__cells = [s.get_name() for s in self.get_symbols() if s.is_cell()] | |
| is_cell = lambda x: _get_scope(x) == CELL | |
| self.__cells = self.__idents_matching(is_cell) |
|
Can you also update symtable.rst? and add a What's New entry? (I did so for what I changed in 3.14) And once you update the docs, update the NEWS entry to point them to |
| return self.__frees | ||
|
|
||
| def get_cells(self): | ||
| """Return a list of cell variable names in the table. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| """Return a list of cell variable names in the table. | |
| """Return a tuple of cell variable names in the table. |
| def test_cells(self): | ||
| self.assertTrue(self.spam.lookup("x").is_cell()) | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There ate 2 blanks instead of 1 here.
Expose cell status in symtable.Symbol via new
is_cell()predicate and addget_cells()method to SymbolTable.Changes
Symbol.is_cell()method that returnsTrueif the symbol has the CELL flag.SymbolTable.get_cells()method that returns an iterable of cell variable names.test_cellstoLib/test/test_symtable.pyto verify the new API.Tests
test_cellstest using a closure pattern (outer -> inneraccessingx).