GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-28754: AC on bisect. by JulienPalard · Pull Request #177 · python/cpython

Conversation

JulienPalard commented Feb 19, 2017 •

edited by methane

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

[bpo-28754]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          issue28754: AC on bisect.

26529c0

the-knights-who-say-ni added the CLA signed label

Feb 19, 2017

JulienPalard changed the title ~~bpo28754: AC on bisect.~~ bpo-28754: AC on bisect.

Feb 19, 2017

methane reviewed

Feb 20, 2017

View reviewed changes

methane left a comment

Copy link

Copy Markdown

Member

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.

LGTM.
But please wait approval from expert.
(@rhettinger is listed on http://cpython-devguide.readthedocs.io/experts.html)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka requested changes

Feb 23, 2017

View reviewed changes

Comment thread

Modules/_bisectmodule.c

		#include "Python.h"

		static int
		ssize_t_converter(PyObject obj, void ptr)

serhiy-storchaka Feb 23, 2017

Copy link

Copy Markdown

Member

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.

ssize_t_converter is not used for parsing arguments. It is used only in optional_ssize_t_converter. Wouldn't be better to inline it?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_bisectmodule.c


		a: object
		x: object
		lo: Py_ssize_t(c_default='0') = 0

serhiy-storchaka Feb 23, 2017

Copy link

Copy Markdown

Member

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.

c_default='0' is redundant.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_bisectmodule.c



		/*[clinic input]
		bisect.bisect_right

serhiy-storchaka Feb 23, 2017

Copy link

Copy Markdown

Member

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.

You can use the return converter for simplifying the code.

bisect.bisect_right -> Py_ssize_t

The body of bisect_bisect_right_impl() would be just:

return internal_bisect_right(a, x, lo, hi);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_bisect.py


		def test_hi_backcompatbility(self):
		for func, data, elem, expected in self.precomputedCases:
		self.assertEqual(func(data, elem, 0, -1),

serhiy-storchaka Feb 23, 2017

Copy link

Copy Markdown

Member

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 doesn't check that passing -1 and None is the same as not specifying the hi argument. Compare results with func(data, elem).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka requested a review from rhettinger

February 23, 2017 13:10

rhettinger self-assigned this

Apr 5, 2017

rhettinger commented Apr 5, 2017

Copy link

Copy Markdown

Contributor

I don't want the pure python API to change at all. It is a long standing API and there is nothing wrong with it. Also, passing it -1 here as a special value makes real code less readable. Further, the change slightly slows down the case where high is specified. In a high performance building block module, we shouldn't add in unnecessary checks.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

brettcannon commented Feb 21, 2018

Copy link

Copy Markdown

Member

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

brettcannon added the awaiting changes label

Feb 21, 2018

JulienPalard closed this

Mar 24, 2018

akruis added a commit to akruis/cpython that referenced this pull request

Nov 1, 2018


          Stackless issue python#177: Fix prickelpit.c function slp_cannot_exec…

5a574b8

…ute()

I missed the C-fame case, when I changed the frame-function ref counting
in Stackless issue python#117.

JulienPalard deleted the issue28754-ac-bisect branch

June 16, 2019 14:05

jaraco pushed a commit that referenced this pull request

Dec 2, 2022


          Update pytest from 3.10.1 to 4.0.0 (#177)

5a43e6c

Reviewers

methane

methane left review comments

serhiy-storchaka

serhiy-storchaka requested changes

rhettinger

Awaiting requested review from rhettinger

Uh oh!

There was an error while loading. Please reload this page.

Assignees

rhettinger

Labels

awaiting changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

JulienPalard commented Feb 19, 2017 • edited by methane Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

methane left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Feb 23, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Feb 23, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Feb 23, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Feb 23, 2017

Choose a reason for hiding this comment

Uh oh!

rhettinger commented Apr 5, 2017

Uh oh!

brettcannon commented Feb 21, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JulienPalard commented Feb 19, 2017 •

edited by methane

Loading

Uh oh!

There was an error while loading. Please reload this page.