Multidict views C ext #275

iemelyanov · 2018-08-16T13:04:14Z

No description provided.

codecov · 2018-08-16T13:46:49Z

Codecov Report

❗ No coverage uploaded for pull request base (master@ca9121d). Click here to learn what that means.
The diff coverage is 73.39%.

@@            Coverage Diff            @@
##             master     #275   +/-   ##
=========================================
  Coverage          ?   93.48%           
=========================================
  Files             ?       10           
  Lines             ?      936           
  Branches          ?        0           
=========================================
  Hits              ?      875           
  Misses            ?       61           
  Partials          ?        0

Impacted Files	Coverage Δ
multidict/_multidict_base.py	`73.39% <73.39%> (ø)`

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 ca9121d...b828f48. Read the comment docs.

multidict/_multidict.pyx

asvetlov · 2018-09-22T15:52:55Z

multidict/_multidict_views.c

+static INLINE void
+_init_view(_Multidict_ViewObject *self, PyObject *md)
+{
+    self->md = md;


Please incref md here and drop increfs from _init_view calls

asvetlov · 2018-09-22T15:55:32Z

multidict/_multidict_views.c

+    PyObject *impl = NULL,
+             *iter = NULL;
+
+    if (self->md == NULL) {


When self->md is NULL?

Anyway even if it could be (broken code?) -- better to raise an exception.
Returning None from __iter__ violates iterator protocol.

This is useless check. I'll remove it.

asvetlov · 2018-09-22T15:56:50Z

multidict/_multidict_views.c

+        Py_RETURN_NONE;
+    }
+
+    impl = _PyObject_CallMethodId(self->md, &PyId_impl, NULL);


check for impl == NULL is needed

The same for any PyObject_Call*

asvetlov · 2018-09-22T16:12:17Z

multidict/_multidict_views.c

+    0,                                             /* tp_as_buffer */
+    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC,       /* tp_flags */
+    0,                                             /* tp_doc */
+    0,                                             /* tp_traverse */


MultiDict stores any objects as values.
As a consequence, it can have circular deps to itself.
This implies tp_traverse and tp_clear slot implementation for all views and iterators.

asvetlov · 2018-09-22T18:20:25Z

multidict/_multidict_views.c

+        return 0;
+    }
+
+    while ((item = PyIter_Next(iter)) != NULL) {


If returned value is NULL you need to check for exception by PyErr_Occuired().
See https://docs.python.org/3/c-api/iter.html?highlight=pyiter_next#c.PyIter_Next for code example.

The same for other PyIter_Next() usages in the file

asvetlov

Everything looks perfect!

Thanks!

asvetlov · 2018-09-23T06:36:15Z

Thanks!

Bumps [towncrier](https://github.com/hawkowl/towncrier) from 19.2.0 to 21.3.0. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/twisted/towncrier/blob/master/NEWS.rst">towncrier's changelog</a>.</em></p> <blockquote> <h1>towncrier 21.3.0 (2021-04-02)</h1> <p>No significant changes since the previous release candidate.</p> <h1>towncrier 21.3.0.rc1 (2021-03-21)</h1> <h2>Features</h2> <ul> <li>Ticket number from file names will be stripped down to avoid ticket links such as <code>[#7](https://github.com/hawkowl/towncrier/issues/007)</code>. (<code>[#126](twisted/towncrier#126) <https://github.com/hawkowl/towncrier/issues/126></code>_)</li> <li>Allow definition of the project <code>version</code> and <code>name</code> in the configuration file. This allows use of towncrier seamlessly with non-Python projects. (<code>[#165](twisted/towncrier#165) <https://github.com/hawkowl/towncrier/issues/165></code>_)</li> <li>Improve news fragment file name parsing to allow using file names like <code>123.feature.1.ext</code> which are convenient when one wants to use an appropriate extension (e.g. <code>rst</code>, <code>md</code>) to enable syntax highlighting. (<code>[#173](twisted/towncrier#173) <https://github.com/hawkowl/towncrier/issues/173></code>_)</li> <li>The new <code>--edit</code> option of the <code>create</code> subcommand launches an editor for entering the contents of the newsfragment. (<code>[#275](twisted/towncrier#275) <https://github.com/hawkowl/towncrier/issues/275></code>_)</li> <li>CPython 3.8 and 3.9 are now part of our automated test matrix and are officially supported. (<code>[#291](twisted/towncrier#291) <https://github.com/hawkowl/towncrier/issues/291></code>_)</li> <li>When searching for the project, first check for an existing importable instance. This helps if the version is only available in the installed version and not the source. (<code>[#297](twisted/towncrier#297) <https://github.com/hawkowl/towncrier/issues/297></code>_)</li> <li>Support building with PEP 517. (<code>[#314](twisted/towncrier#314) <https://github.com/hawkowl/towncrier/issues/314></code>_)</li> </ul> <h2>Bugfixes</h2> <ul> <li>Configuration errors found during command line execution now trigger a message to stderr and no longer show a traceback. (<code>[#84](twisted/towncrier#84) <https://github.com/hawkowl/towncrier/issues/84></code>_)</li> <li>A configuration error is triggered when the newsfragment files couldn't be discovered. (<code>[#85](twisted/towncrier#85) <https://github.com/hawkowl/towncrier/issues/85></code>_)</li> <li>Invoking towncrier as <code>python -m towncrier</code> works. (<code>[#163](twisted/towncrier#163) <https://github.com/hawkowl/towncrier/issues/163></code>_)</li> <li><code>check</code> subcommand defaults to UTF-8 encoding when <code>sys.stdout.encoding</code> is <code>None</code>. This happens, for example, with Python 2 on GitHub Actions or when the output is piped. (<code>[#175](twisted/towncrier#175) <https://github.com/hawkowl/towncrier/issues/175></code>_)</li> <li>Specifying <code>title_format</code> disables default top line creation to avoid duplication. (<code>[#180](twisted/towncrier#180) <https://github.com/hawkowl/towncrier/issues/180></code>_)</li> </ul> <h2>Improved Documentation</h2> <ul> <li>The README now mentions the possibility to name the configuration file <code>towncrier.toml</code> (in addition to <code>pyproject.toml</code>). (<code>[#172](twisted/towncrier#172) <https://github.com/hawkowl/towncrier/issues/172></code>_)</li> <li><code>start_line</code> corrected to <code>start_string</code> in the readme to match the long standing implementation. (<code>[#277](twisted/towncrier#277) <https://github.com/hawkowl/towncrier/issues/277></code>_)</li> </ul> <h1>towncrier 19.9.0 (2021-03-20)</h1> <p>No significant changes.</p>  </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/twisted/towncrier/commit/eab34611b93a4ba6e3805cd546a674d88dbd43cf"><code>eab3461</code></a> Update NEWS.rst</li> <li><a href="https://github.com/twisted/towncrier/commit/bd963c351853d8977cd37c501b2b1b85d7cb0217"><code>bd963c3</code></a> add back the newsfragment...</li> <li><a href="https://github.com/twisted/towncrier/commit/3bf4479562f0cc6ff828fe93fffb94fadc4c731d"><code>3bf4479</code></a> Bump version to 21.3.0</li> <li><a href="https://github.com/twisted/towncrier/commit/3237ebd09527cecfd758c0fb472a3f5c7ccb1acb"><code>3237ebd</code></a> remove misc section</li> <li><a href="https://github.com/twisted/towncrier/commit/6eab89476cd4b3849563496c19d07905109c9f61"><code>6eab894</code></a> Correct underlines setting to toml in readme</li> <li><a href="https://github.com/twisted/towncrier/commit/4143e0ff55177cca5747d18624d614e593ebe3c3"><code>4143e0f</code></a> add 332.misc</li> <li><a href="https://github.com/twisted/towncrier/commit/05eea3f2240bfebee4522e1c1180893f7a79a509"><code>05eea3f</code></a> Release 21.3.0rc1</li> <li><a href="https://github.com/twisted/towncrier/commit/2841c12f2e7ebfc40d9cc3228ca468d339225549"><code>2841c12</code></a> Release 19.9.0 (<a href="https://github-redirect.dependabot.com/hawkowl/towncrier/issues/331">#331</a>)</li> <li><a href="https://github.com/twisted/towncrier/commit/7267fb56e2a34b4f81b4e0005e3f4eeddaa71b2b"><code>7267fb5</code></a> twisted-alike (so similar) automatic publishing on gha (<a href="https://github-redirect.dependabot.com/hawkowl/towncrier/issues/315">#315</a>)</li> <li><a href="https://github.com/twisted/towncrier/commit/499c8f7990f82d7decd32f139ec7a1c2d1b25719"><code>499c8f7</code></a> use incremental to canonicalize versions for release (<a href="https://github-redirect.dependabot.com/hawkowl/towncrier/issues/329">#329</a>)</li> <li>Additional commits viewable in <a href="https://github.com/hawkowl/towncrier/compare/19.2.0...21.3.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=towncrier&package-manager=pip&previous-version=19.2.0&new-version=21.3.0)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end)

iemelyanov added 27 commits August 8, 2018 01:32

Initial commit multidict_views.

61ec3d8

Add repr for _ItemsView.

3a3e1db

Refactoring.

ba67391

Multidict base view methods for colling from C extension.

f17b398

Remove cython _ItemsView.

6a49080

Impl all C ItemsView base methods.

4347bb8

Add KeysView repr for calling from C.

daa949d

Add KeysView isdisjoint for calling from C.

a255550

Refactoring names.

d6b605d

Add ItemsView isdisjoint for calling from C.

7770cad

Impl call ItemsView isdisjoint from python.

445c2dc

Add comment.

eda19f0

Rm unused code.

216bb40

Rm unused preprocessor.

da79aa1

Fix names.

3b7bbf8

Make call impl method faster.

1b2f246

Impl contains for KeysView.

97e988b

Impl repr for KeysView.

0a0efb7

Impl isdisjoint for KeysView. Refactoring.

3300af4

Intergrate C KeysView to cython. Refactoring.

e206c90

Add ValuesView repr for calling from C.

48f7e25

Impl repr for ValuesView.

a86946f

Impl contains for ValuesView.

108e814

Add base methods for ValuesView.

6eb3baf

Intergrate C ValuesView to cython. Refactoring.

00d2d27

Fix some mem leak. Add todo comments.

8fa85c0

Fix c compiler flag.

23f9b4a

iemelyanov requested review from asvetlov and webknjaz as code owners August 16, 2018 13:04

Fix style.

f2e101e

webknjaz removed their request for review August 16, 2018 13:09

Make microsoft c89 happy.

f7a5e67

Remove some TODO.

be03d7c

asvetlov requested changes Sep 22, 2018

View reviewed changes

Fix PR issues.

cbf17b6

asvetlov reviewed Sep 22, 2018

View reviewed changes

Fix PR issues.

975cc49

asvetlov approved these changes Sep 22, 2018

View reviewed changes

Add tp_clear and little refactoring.

b828f48

asvetlov merged commit 08fe793 into aio-libs:master Sep 23, 2018

byashimov mentioned this pull request Nov 26, 2018

multidict 4.5 memory leak #307

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multidict views C ext #275

Multidict views C ext #275

iemelyanov commented Aug 16, 2018

codecov bot commented Aug 16, 2018 •

edited

Loading

asvetlov Sep 22, 2018

iemelyanov Sep 22, 2018

asvetlov Sep 22, 2018

iemelyanov Sep 22, 2018

asvetlov Sep 22, 2018

asvetlov Sep 22, 2018

iemelyanov Sep 22, 2018

asvetlov Sep 22, 2018

iemelyanov Sep 22, 2018

asvetlov Sep 22, 2018

iemelyanov Sep 22, 2018

asvetlov left a comment

asvetlov commented Sep 23, 2018

Multidict views C ext #275

Multidict views C ext #275

Conversation

iemelyanov commented Aug 16, 2018

codecov bot commented Aug 16, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvetlov left a comment

Choose a reason for hiding this comment

asvetlov commented Sep 23, 2018

codecov bot commented Aug 16, 2018 •

edited

Loading