bpo-30486: Allow setting cell value by lisroach · Pull Request #1840 · python/cpython

lisroach · 2017-05-28T05:00:14Z

No description provided.

mention-bot · 2017-05-28T05:00:15Z

@lisroach, thanks for your PR! By analyzing the history of the files in this pull request, we identified @warsaw, @Yhg1s and @tiran to be potential reviewers.

codecov · 2017-05-28T05:42:50Z

Codecov Report

Merging #1840 into master will decrease coverage by 1.02%.
The diff coverage is 94.44%.

@@            Coverage Diff             @@
##           master    #1840      +/-   ##
==========================================
- Coverage   83.68%   82.65%   -1.03%     
==========================================
  Files        1371     1432      +61     
  Lines      347053   353644    +6591     
==========================================
+ Hits       290428   292307    +1879     
- Misses      56625    61337    +4712

Impacted Files	Coverage Δ
Lib/test/test_funcattrs.py	`92.55% <94.44%> (+0.12%)`	⬆️
Lib/lib2to3/fixes/fix_ws_comma.py	`41.66% <0%> (-58.34%)`	⬇️
Lib/lib2to3/tests/test_all_fixers.py	`66.66% <0%> (-25%)`	⬇️
Lib/lib2to3/pytree.py	`84.91% <0%> (-5.84%)`	⬇️
Lib/lib2to3/tests/support.py	`87.09% <0%> (-3.23%)`	⬇️
Lib/test/test_io.py	`94.97% <0%> (-1.98%)`	⬇️
Lib/test/test_faulthandler.py	`93.85% <0%> (-1.22%)`	⬇️
Lib/logging/handlers.py	`83.38% <0%> (-0.53%)`	⬇️
Lib/test/datetimetester.py	`95.84% <0%> (-0.48%)`	⬇️
Lib/lib2to3/refactor.py	`80.17% <0%> (-0.44%)`	⬇️
... and 92 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 178418a...bd5e329. Read the comment docs.

rhettinger

Please add tests and documentation updates.

rhettinger · 2017-05-28T07:03:47Z

+cell_set_contents(PyCellObject *op, PyCellObject *obj)
+{
+    Py_INCREF(obj);
+    PyCellObject *tmp_op = (PyCellObject *)(op->ob_ref);


I believe the 147 to 149 can be replace by a single invocation of the Py_SETREF macro. See +856 in Include/object.h.

This is what I originally had in my first commit, but running make patchcheck showed this warning:
incompatible pointer types assigning to 'PyObject *' (aka 'struct _object *') from 'PyCellObject *'

Which is why I rewrote with the current lines. What do you think?

What about

Py_XSETREF(op->ob_ref, (PyObject *)obj);

?

You may have used the macro incorrectly. Did you try Py_SETREF(op->ob_ref, obj)?
Actually, you need Py_XSETREF (and Py_XINCREF), since obj can be NULL as @serhiy-storchaka pointed out.

@serhiy-storchaka is right, the correct syntax is
Py_XSETREF(op->ob_ref, (PyObject *)obj);

But after looking at it more I obj should be a PyObject anyway and not a PyCellObject. I am new at C though so I could be wrong!

serhiy-storchaka

In addition to requested code changes, needed tests and an entry in Misc/NEWS (maybe even in What's New).

serhiy-storchaka · 2017-05-28T08:02:56Z

+int
+cell_set_contents(PyCellObject *op, PyCellObject *obj)
+{
+    Py_INCREF(obj);


obj can be NULL (when delete the attribute: del cell.cell_contents).

serhiy-storchaka · 2017-06-01T05:22:40Z

+int
+cell_set_contents(PyCellObject *op, PyObject *obj)
+{
+    if (obj == Py_None)


None is a legal value for the cell value.

serhiy-storchaka · 2017-06-01T05:23:59Z

+        c = f.__closure__
+        c[0].cell_contents = 9
+        self.assertEqual(c[0].cell_contents, 9)
+        c[0].cell_contents = None


None is a legal value for the cell value.

You should test

del c[0].cell_contents

serhiy-storchaka · 2017-06-01T05:27:20Z

      |                         | attributes.                   |           |
      +-------------------------+-------------------------------+-----------+
-      | :attr:`__closure__`     | ``None`` or a tuple of cells  | Read-only |
+      | :attr:`__closure__`     | ``None`` or a tuple of cells  | Writable  |


The __closure__ attribute is not writable. The undocumented cell_contents attribute of __closure__ elements is writable.

pitrou · 2017-06-02T16:42:38Z


+    def test_set_cell(self):
+        a = 12
+        def f(): print(a)


If you use def f(): return a instead, you could verify that calling f() does return the updated value.

It's not a blocker to getting this accepted, btw, just nice to have ;-)

That is a good idea!

serhiy-storchaka · 2017-06-03T15:56:00Z

+        def f(): print(a)
+        c = f.__closure__
+        c[0].cell_contents = 9
+        self.assertEqual(c[0].cell_contents, 9)


It would be nice to check how setting and deleting cell_contents affects the local variable a.

serhiy-storchaka · 2017-06-03T16:02:59Z

      |                         | function's free variables.    |           |
+      |                         | The ``cell_contents``         |           |
+      |                         | attribute can be used for     |           |
+      |                         | used for reading and writing  |           |


Duplicated "used for".

It may be better to describe the cell object somewhere else (for example in a footnote or in a separate paragraph following this table) than in a narrow table cell.

It is kind of awkward trying to fit it in the table, I'll try to put it somewhere below.

serhiy-storchaka · 2017-06-06T04:19:36Z

      can be used, for example, to attach metadata to functions.  Regular attribute
      dot-notation is used to get and set such attributes. *Note that the current
      implementation only supports function attributes on user-defined functions.
-      Function attributes on built-in functions may be supported in the future.*


Removed * (it needs for ending an emphasizing of a note).

Should we remove the * in front of Note as well?

Just restore removed * here.

Oh oops, sorry I didn't realize I had deleted that!

serhiy-storchaka · 2017-06-06T04:22:43Z

+        c = f.__closure__
+        c[0].cell_contents = 9
+        self.assertEqual(c[0].cell_contents, 9)
+        self.assertEqual(f(), 9)


Also check a directly.

self.assertEqual(a, 9)

serhiy-storchaka · 2017-06-06T04:25:47Z

+        self.assertEqual(c[0].cell_contents, 9)
+        self.assertEqual(f(), 9)
+        del c[0].cell_contents
+        try:


I understand that you have copied an example from the above test, but this is too verbose. with self.assertRaises(ValueError): is much more compact.

serhiy-storchaka · 2017-06-07T04:01:39Z

        else:
            self.fail("shouldn't be able to read an empty cell")
-        try:
+        with self.assertRaises(NameError):


Also check a directly.

lisroach added 2 commits May 27, 2017 19:43

Adds cell_set_contents.

83b5e62

Not using Py_SETREF bc it uses PyObject instead of PyCellObject.

9b4cfd2

the-knights-who-say-ni added the CLA signed label May 28, 2017

rhettinger requested changes May 28, 2017

View reviewed changes

serhiy-storchaka requested changes May 28, 2017

View reviewed changes

Mariatta added the type-feature A feature request or enhancement label May 29, 2017

Updates to use XSETREF. Adds docs, tests, and news update.

244cb7e

serhiy-storchaka requested changes Jun 1, 2017

View reviewed changes

cell_contents can be set to none. Update documentation.

c5db387

pitrou reviewed Jun 2, 2017

View reviewed changes

serhiy-storchaka reviewed Jun 3, 2017

View reviewed changes

Updates test with return value and updates documentation.:

02dbd0e

serhiy-storchaka reviewed Jun 6, 2017

View reviewed changes

Updates tests.

447cc10

serhiy-storchaka reviewed Jun 7, 2017

View reviewed changes

lisroach added 3 commits June 6, 2017 21:35

Checks a value after deletion.

b22b945

removes trailing whitespace from docs.

f11ace7

Adds back in *.

bd5e329

serhiy-storchaka approved these changes Jun 8, 2017

View reviewed changes

Merge branch 'master' into issue30486

1f0ef6c

serhiy-storchaka merged commit 64505a1 into python:master Jun 8, 2017

Uh oh!

Uh oh!

Conversation

lisroach commented May 28, 2017

Uh oh!

mention-bot commented May 28, 2017

Uh oh!

codecov Bot commented May 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rhettinger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov Bot commented May 28, 2017 •

edited

Loading