Skip to content

fix(qgsthreadingutils): format with clang to improve readability #62527

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

benoitdm-oslandia
Copy link
Collaborator

@benoitdm-oslandia benoitdm-oslandia commented Jul 7, 2025

The preview in github is pretty ugly... better look directly at the file: https://github.com/benoitdm-oslandia/QGIS/blob/fix/qgsthreadingutils/src/core/qgsthreadingutils.h

@benoitdm-oslandia benoitdm-oslandia self-assigned this Jul 7, 2025
@github-actions github-actions bot added this to the 3.46.0 milestone Jul 7, 2025
Copy link
Contributor

github-actions bot commented Jul 7, 2025

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 16a947a)

🍎 MacOS Qt6 builds

Download MacOS Qt6 builds of this PR for testing.
This installer is not signed, control+click > open the app to avoid the warning
(Built from commit 16a947a)

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 16a947a)

@benoitdm-oslandia
Copy link
Collaborator Author

@nyalldawson I had applied the formatting to qgslogger too.

@benoitdm-oslandia
Copy link
Collaborator Author

It is strange as sipify breaks the build after I formatted qgslogger. I had to remove the bad parts manually in the generated files. May be I should remove the modifications on qgslogger but I still do not understand what the issue here!

@troopa81 a suggestion?

@troopa81
Copy link
Contributor

Isn't that just your forget to run sipify ? because as far as I remember sip_check just run the sipify script and diff with the current pushed files.

@benoitdm-oslandia
Copy link
Collaborator Author

@troopa81 I have ran a sipify_all.sh to be sure and it failed at build time 😞

@benoitdm-oslandia
Copy link
Collaborator Author

rebase from master and fixed bad sip file generation by adding #ifndef SIP_RUN

@@ -23,8 +23,10 @@

#include "qgsfeedback.h"

#ifndef SIP_RUN
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this if we have already SIP_NO_FILE ?

Copy link
Collaborator Author

@benoitdm-oslandia benoitdm-oslandia Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because sipify.py still parsed the file and crashed due to the '\' at the end of lines. I remove the #define SIP_NO_FILE

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI crashed ... both are needed!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not fix sip_include.sh so it doesn't include this file when it meets sip_no_file ? Is this complicated to fix ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried but I am not proud of what I have done. Can you check please?

@@ -3682,6 +3682,23 @@ def generate_python_output():
)
sys.exit(1)

# early handle of SIP_NO_FILE directive to avoid useless sip generation
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is wrong -- we should be catching this earlier, in sip_include.sh, or in the pre-commit scripts themselves.

@benoitdm-oslandia benoitdm-oslandia marked this pull request as draft July 21, 2025 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants