Skip to content
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

Fix formatting of dlopen error message on Windows #78802

Conversation

KamilBrzoskowski
Copy link
Contributor

When developing GDExtension if there is a problem with dll loading you can get "not enough arguments for format string".
Reason for that is rather than using p_path as a parameter it is used to add to string, which causes vformat function to get one less parameter and sprintf to return error down the line.

@KamilBrzoskowski KamilBrzoskowski requested a review from a team as a code owner June 28, 2023 16:04
@YuriSizov YuriSizov changed the title Fixed vFormat by changing "+" to ",", only for os_windows.cpp Fix incorrect number of arguments in a vformat call on Windows Jun 28, 2023
@YuriSizov YuriSizov added this to the 4.2 milestone Jun 28, 2023
@YuriSizov YuriSizov added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jun 28, 2023
@YuriSizov
Copy link
Contributor

Good catch! Looks like a typo when someone converted the old string-concatenated message to vformat. Other platforms still use string concatenation, so it would be cool to change them to use vformat as well, and unify the message, since there are slight differences.

@akien-mga akien-mga changed the title Fix incorrect number of arguments in a vformat call on Windows Fix formatting of dlopen error message on Windows Jul 9, 2023
And harmonize the format for all platforms.

Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>
@akien-mga akien-mga force-pushed the fix-vformat-for-os_windows-file branch from 6da7171 to 3cd865d Compare July 9, 2023 21:40
@akien-mga akien-mga requested review from a team as code owners July 9, 2023 21:40
@akien-mga
Copy link
Member

Other platforms still use string concatenation, so it would be cool to change them to use vformat as well, and unify the message, since there are slight differences.

I pushed an update to do that.

@akien-mga akien-mga merged commit 0bf8261 into godotengine:master Jul 9, 2023
13 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jul 10, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.1.

@KamilBrzoskowski KamilBrzoskowski deleted the fix-vformat-for-os_windows-file branch July 11, 2023 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants