GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-1635741 port _lsprof to multi-phase init (PEP 489) by koubaa · Pull Request #22220 · python/cpython

Conversation

@koubaa

@koubaa koubaa commented Sep 12, 2020 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue1635741

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Sep 12, 2020
@bedevere-bot bedevere-bot added the awaiting review label Sep 12, 2020
@koubaa

koubaa commented Sep 13, 2020

Copy link
Copy Markdown
Contributor Author

@vstinner @corona10 @shihai1991 please review
All reactions

Sorry, something went wrong.

Uh oh!

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

shihai1991 commented Sep 13, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

compile warning: ‘getstats_doc’ defined but not used [-Wunused-const-variable=
those code should be removed?
All reactions

Sorry, something went wrong.

Uh oh!

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

koubaa commented Sep 13, 2020

Copy link
Copy Markdown
Contributor Author

compile warning: ‘getstats_doc’ defined but not used [-Wunused-const-variable=
those code should be removed?

Oops, I meant to remove that!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Sep 14, 2020
View reviewed changes
Comment thread Modules/_lsprof.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@koubaa koubaa mentioned this pull request Sep 14, 2020
bpo-1635741: convert an _lsprof method to argument clinic #22240
Merged
@koubaa
koubaa force-pushed the bpo-1635741-lsprof branch from ae2e03e to d4800d3 Compare September 21, 2020 23:28
koubaa added 2 commits September 21, 2020 18:29
@koubaa
convert lsprof to multi-phase init
c818a4e
@koubaa
blurb
1a01fed
@koubaa
koubaa force-pushed the bpo-1635741-lsprof branch from d4800d3 to 1a01fed Compare September 21, 2020 23:29
vstinner
vstinner approved these changes Sep 22, 2020
View reviewed changes

@vstinner vstinner 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 I have some coding style requests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_lsprof.c Outdated
static PyStructSequence_Desc profiler_entry_desc = {
"_lsprof.profiler_entry", /* name */
NULL, /* doc */
"", /* doc */

@vstinner vstinner Sep 22, 2020

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.

IMO PyType_FromSpec() should accept tp_doc=NULL, I created: https://bugs.python.org/issue41832

I don't ask you to change your PR, "" vs NULL can be changed again later.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_lsprof.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_lsprof.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/_lsprof.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Sep 22, 2020
@koubaa
update based on review
ce40825
vstinner
vstinner approved these changes Sep 23, 2020
View reviewed changes

@vstinner vstinner 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. Thanks, the new code is easier to read!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_lsprof.c
.name = "_lsprof.profiler_entry",
.doc = "",
.fields = profiler_entry_fields,
.n_in_sequence = 6

@vstinner vstinner Sep 23, 2020

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.

For me, it's way easier to read than the old format, thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/_lsprof.c
if (PyModule_AddType(module, state->stats_subentry_type) < 0) {
return -1;
}

@vstinner vstinner Sep 23, 2020

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.

Thanks, the new code is more regular (create type, add type, create type, add type, ...) and so easier to read!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner
vstinner merged commit 83de110 into python:master Sep 23, 2020
@bedevere-bot bedevere-bot removed the awaiting merge label Sep 23, 2020
shihai1991 added a commit to shihai1991/cpython that referenced this pull request Sep 24, 2020
@shihai1991
Merge remote-tracking branch 'origin/master' into bpo_39337_new_3
e29ec44 
* origin/master: (27 commits)
  bpo-41428: Fix compiler warnings in unionobject.c (pythonGH-22388)
  bpo-41654: Fix compiler warning in MemoryError_dealloc() (pythonGH-22387)
  bpo-41833: threading.Thread now uses the target name (pythonGH-22357)
  bpo-30155: Add macros to get tzinfo from datetime instances (pythonGH-21633)
  bpo-33822: Update IDLE section of What's New 3.8 (pythonGH-22383)
  bpo-41844: Add IDLE section to What's New 3.9 (GN-22382)
  bpo-41841: Prepare IDLE News for 3.10 (pythonGH-22379)
  bpo-37779 : Add information about the overriding behavior of ConfigParser.read (pythonGH-15177)
  bpo-40170: Use inline _PyType_HasFeature() function (pythonGH-22375)
  bpo-40941: Fix stackdepth compiler warnings (pythonGH-22377)
  bpo-40941: Fix fold_tuple_on_constants() compiler warnings (pythonGH-22378)
  bpo-40521: Fix PyUnicode_InternInPlace() (pythonGH-22376)
  bpo-41834: Remove _Py_CheckRecursionLimit variable (pythonGH-22359)
  bpo-1635741, unicodedata: add ucd_type parameter to UCD_Check() macro (pythonGH-22328)
  bpo-1635741: Port _lsprof extension to multi-phase init (PEP 489) (pythonGH-22220)
  bpo-41513: Improve order of adding fractional values. Improve variable names. (pythonGH-22368)
  bpo-41816: `StrEnum.__str__` is `str.__str__` (pythonGH-22362)
  bpo-35764: Rewrite the IDLE Calltips doc section  (pythonGH-22363)
  bpo-41810: Reintroduce `types.EllipsisType`, `.NoneType` & `.NotImplementedType` (pythonGH-22336)
  bpo-41602: raise SIGINT exit code on KeyboardInterrupt from pymain_run_module (python#21956)
  ...
xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
@koubaa @xzy3-cdc
bpo-1635741: Port _lsprof extension to multi-phase init (PEP 489) (py…
499db01 
…thonGH-22220)
@shihai1991 shihai1991 mentioned this pull request Nov 3, 2020
bpo-41832: PyType_FromModuleAndSpec() can accept the NULL tp_doc slot. #23123
Merged
@kylotan kylotan mannequin mentioned this pull request Sep 19, 2022
Py_Finalize() doesn't clear all Python objects at exit #44470
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

5 participants

@koubaa @shihai1991 @vstinner @the-knights-who-say-ni @bedevere-bot
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.