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 — Allow local use of `static PyMutex` in the C analyzer by ZeroIntensity · Pull Request #127102 · python/cpython

Conversation

@ZeroIntensity

@ZeroIntensity ZeroIntensity commented Nov 21, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

As seen with gh-127055, the C analyzer doesn't like static local variables, but it's fine to do so for a PyMutex. If we want to use a similar approach for other libc functions (as mentioned in gh-127081), it would be better to gracefully handle this instead of adding to ignored.tsv every time.

Sorry, something went wrong.

Uh oh!

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

All reactions
@ZeroIntensity
Allow use of local static mutexes in the C analyzer.
6c7830e
@ZeroIntensity ZeroIntensity added skip issue skip news labels Nov 21, 2024
@ZeroIntensity
ZeroIntensity requested a review from ericsnowcurrently as a code owner November 21, 2024 14:33
@bedevere-app bedevere-app Bot added the awaiting review label Nov 21, 2024
@ZeroIntensity
Remove debug.
614e923
@vstinner

vstinner commented Nov 21, 2024

Copy link
Copy Markdown
Member

Can you rebase your PR on the main branch and remove:

diff --git a/Tools/c-analyzer/cpython/ignored.tsv b/Tools/c-analyzer/cpython/ignored.tsv
index 4327a111eed..686f3935d91 100644
--- a/Tools/c-analyzer/cpython/ignored.tsv
+++ b/Tools/c-analyzer/cpython/ignored.tsv
@@ -739,7 +739,6 @@ Modules/expat/xmlrole.c     -       declClose       -
 Modules/expat/xmlrole.c        -       error   -
 
 ## other
-Modules/grpmodule.c    grp_getgrall_impl       getgrall_mutex  -
 Modules/_io/_iomodule.c        -       _PyIO_Module    -
 Modules/_sqlite/module.c       -       _sqlite3module  -
 Modules/clinic/md5module.c.h   _md5_md5        _keywords       -
All reactions

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently
ericsnowcurrently requested changes Nov 21, 2024
View reviewed changes

@ericsnowcurrently ericsnowcurrently 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.

Would you mind removing any corresponding variables from ignored.tsv (and/or globals-to-fix.tsv), like the one added in gh-127055? That would help verify that this change is working right.

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-app bedevere-app Bot added awaiting changes and removed awaiting review labels Nov 21, 2024
@bedevere-app

bedevere-app Bot commented Nov 21, 2024

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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

ZeroIntensity added 2 commits November 21, 2024 11:31
@ZeroIntensity
Merge branch 'main' into static-mutex-in-analyzer
ece60e3
@ZeroIntensity
Remove no longer needed ignore
61ed5f6
@ZeroIntensity

ZeroIntensity commented Nov 21, 2024

Copy link
Copy Markdown
Member Author

Ok, the ignore from Victor's PR has been removed. I'll do a more thorough audit of what needs to get un-ignored when I'm more available later today.

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner
vstinner approved these changes Nov 21, 2024
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

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting changes labels Nov 21, 2024
@ZeroIntensity

ZeroIntensity commented Nov 21, 2024

Copy link
Copy Markdown
Member Author

Hmm, this seems like a bedevere bug. This should still have awaiting changes per Eric's review, not awaiting merge.

All reactions

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently
ericsnowcurrently approved these changes Nov 21, 2024
View reviewed changes

@ericsnowcurrently ericsnowcurrently 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

Sorry, something went wrong.

Uh oh!

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

All reactions
@ZeroIntensity

ZeroIntensity commented Nov 22, 2024

Copy link
Copy Markdown
Member Author

Did a search, and it looks like no other mutexes need to get un-ignored. We're good to merge.

ericsnowcurrently reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@vstinner
vstinner merged commit 89125e9 into python:main Nov 22, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Nov 22, 2024
@vstinner

vstinner commented Nov 22, 2024

Copy link
Copy Markdown
Member

Merged, thanks.

ZeroIntensity reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

@ZeroIntensity
ZeroIntensity deleted the static-mutex-in-analyzer branch November 22, 2024 11:20
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@ZeroIntensity @ebonnal
Allow local use of static PyMutex in the C analyzer (python#127102)
f4d7d18
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
@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

skip issue skip news

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.

3 participants

@ZeroIntensity @vstinner @ericsnowcurrently
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 01402520 — 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.