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

SDL_snprintf unit test failing #4762

Closed
Markvy opened this issue Sep 19, 2021 · 17 comments
Closed

SDL_snprintf unit test failing #4762

Markvy opened this issue Sep 19, 2021 · 17 comments
Assignees
Milestone

Comments

@Markvy
Copy link

Markvy commented Sep 19, 2021

The SDL unit tests have a test that basically does this:

int count  = SDL_snprintf(text, 2, "%s", "foo"); // count should be 3 despite truncation

I tried to run that test today on Windows (which is one of the platforms which uses SDL's homemade snprintf rather than delegating to stdio) and the test failed: count is 1 rather than 3.

I assume it's been broken for quite a while:

  1. fresh checkout of the code (as of yesterday) gives me a broken test
  2. using a released build for version 2.0.12 gives me the same result: count = 1

For a bit of back story, see this issue for the wiki I filed yesterday:
libsdl-org/sdlwiki#132
After I filed that, I thought to myself "how hard would it be implement %g anyway?"
To get my feet wet, I tried to check out the code and run the unit tests; to my surprise they failed.
(By the way, README-visualc.md is great! If all SDL docs were that good, life would be grand.)

@ccawley2011
Copy link
Contributor

For reference, this same issue prevents PR #4746 from being merged since SDL_snprintf doesn't return the correct size to allow the buffer to be resized.

@Markvy
Copy link
Author

Markvy commented Sep 19, 2021

Thanks for the extra context! So... what happens now?

@icculus icculus added this to the 2.0.18 milestone Sep 20, 2021
@icculus
Copy link
Collaborator

icculus commented Sep 20, 2021

What happens now is I put this in the 2.0.18 milestone so we don't forget to look at it in the near future. :)

@icculus
Copy link
Collaborator

icculus commented Sep 20, 2021

(That said, the goal here is going to be to fix snprintf and not the unit test, since all of the unit tests are unmaintained.)

@Markvy
Copy link
Author

Markvy commented Sep 21, 2021

Not sure what you mean by "no plan to fix the unit test". The test doesn't need fixing: it is already correct.
It's probably been failing since the day it was written, but it is still correct. Or am I confused?

@slouken slouken self-assigned this Sep 22, 2021
@icculus
Copy link
Collaborator

icculus commented Sep 22, 2021

The test doesn't need fixing: it is already correct.

Sorry, I saw the phrase "broken test" and didn't read carefully. :)

@slouken
Copy link
Collaborator

slouken commented Sep 22, 2021

Also, just my 2 cents, testautomation is pretty valuable, and we should keep it maintained. I already fixed a couple of bugs based on the output.

@Markvy
Copy link
Author

Markvy commented Sep 25, 2021

By the way, no one has commented yet on the issue I opened in the wiki repo about SDL_log.
Is there any plan to do anything about the %g situation, either with actually supporting %g or with documenting how things stand?

@slouken
Copy link
Collaborator

slouken commented Sep 26, 2021

I haven't heard of anyone needing %g before. I don't have time to add it myself, but if you'd like to add support feel free to submit a PR or a patch.

Cheers,

@Markvy
Copy link
Author

Markvy commented Sep 26, 2021

Okay, but I might need some hand-holding. When I checked out the latest SDL from the SDL-mirror repo on GitHub, everything seemed to "just work". When I tried the same thing with this repo, I got a zillion compiler errors and stuff. Should I just file issues as I come across them?

(Also: any particular reason SDL doesn't simply delegate to stdlib when using VC++?)

@slouken
Copy link
Collaborator

slouken commented Sep 27, 2021

Yes, please file bugs for any errors you're getting. We're building here with Visual Studio 2013 and 2019 with no problems.

SDL doesn't use stdlib because games use different versions of the Visual Studio C++ runtime and we don't want to introduce a dependency on a different version.

@slouken
Copy link
Collaborator

slouken commented Sep 27, 2021

You are of course always welcome to enable stdlib in the version of SDL you ship with your game. We just want the default build to be something anyone can drop in anywhere and it should just work.

@Markvy
Copy link
Author

Markvy commented Sep 27, 2021

I think I get it. You can't delegate to stdlib for VC++ without implicitly depending on a SPECIFIC version of it. There's no convenient way for you to say "we want to link against whatever snprintf is available". Or is that not what you meant?

@slouken
Copy link
Collaborator

slouken commented Sep 27, 2021

Yep, that's right.

@Markvy
Copy link
Author

Markvy commented Sep 28, 2021

Two things. First: I grabbed the latest code and most of the syntax errors are gone. Filed an issue for the last one and looks that got fixed yesterday.

Second: looked again at the code I realized I completely misunderstood it the first time.
I was looking at SDL_config_windows.h and I saw that the "HAVE_LIBC" block doesn't define HAVE_VSNPRINTF.
What I completely missed is that this is by default irrelevant, because HAVE_LIBC is not defined by default.
So SDL is more serious than I realized about not touching libc on Windows than I thought.
I'm going to carefully avoid asking more questions about this because I fear the answer will involve the words ABI, and I dare not go there :)

@slouken
Copy link
Collaborator

slouken commented Sep 28, 2021

We're very careful not to have an ABI change regardless of what C runtime the SDL library use. In theory this means that an SDL DLL compiled in one environment can be dropped into a game compiled in another environment, and that's intentional. :)

@Markvy
Copy link
Author

Markvy commented Sep 29, 2021

Impressive :)

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

No branches or pull requests

4 participants