-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Pass argc as reference to reinitializeAsQApplication (fixes #3367) #3370
Conversation
src/main.cpp
Outdated
@@ -117,7 +117,7 @@ void configureApp(bool gui) | |||
|
|||
// TODO find a way so we don't have to do this | |||
/// Recreate the application as a QApplication | |||
void reinitializeAsQApplication(int argc, char* argv[]) | |||
void reinitializeAsQApplication(int &argc, char* argv[]) |
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.
@marceldev89 Nice catch. Though it looks to me that argv
could conceivably fail in the same way. Can you replace it with char *(&argv)[]
?
Edit: Nevermind, it's not necessary based on the signature of the QApplication constructor.
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.
@veracioux it shouldn't. The problem was the QApplication
constructor signature which is QApplication(int &argc, char **argv)
. So it expects a reference to argc
instead of the value. argv
should be fine as char* argv[]
is basically the same as char **argv
I think :)
Edit: haha missed your edit, but yes 😛
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.
@marceldev89 Yup, very subtle and annoying. Please do a clang-format on the code, so our pipeline can pass before merge. Or just moving the &
next to the int
will probably do.
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.
Done :)
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.
Thank you! Merging.
…-org#3367) (flameshot-org#3370) * Pass argc as reference to reinitializeAsQApplication (fixes flameshot-org#3367) * Formatting (cherry picked from commit f54d4c7)
Fixes crash caused by #3313