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

For every variadic function, add a va_list function to the public api #10907

Closed
wants to merge 12 commits into from

Conversation

madebr
Copy link
Contributor

@madebr madebr commented Sep 20, 2024

By providing these va_list alternatives, SDL_dynapi.c can be simplified.
It should also help #9580.

What changed:
SDL_dynapi_proc.h now uses 3 macros:

  • SDL_DYNAPI_PROC: behavior does not change
  • SDL_DYNAPI_VPROC: variadic function returning something
  • SDL_DYNAPI_VOID_VPROC: variadic function returning void

The *_VPROC macros accept 2 additional arguments: forwardfn and last_arg. This is the va_list ap function to which it should forward its arguments. The last_arg is used as 2nd argument to va_start.

I simply appended a V to the va_list alternatives.

Homework for slouken and icculus:

  • come up with better names for the macros and functions
  • review whether my changes to the headers are good, and orderered okay (perhaps you want to move all V declarations to the bottom)

@slouken slouken added this to the 3.0 ABI milestone Sep 20, 2024
@slouken
Copy link
Collaborator

slouken commented Sep 20, 2024

My initial reaction is that it adds a bunch of entry points to the API and doesn't actually reduce or simplify much code, it just moves the complexity around. In addition, SDL_SetError() is now slightly slower, which is probably okay, but not a great selling point.

@icculus might be super happy about it though, so I'll defer to him. :)

src/dynapi/SDL_dynapi.c Outdated Show resolved Hide resolved
@madebr
Copy link
Contributor Author

madebr commented Sep 20, 2024

In addition, SDL_SetError() is now slightly slower, which is probably okay, but not a great selling point.

I see the contrary on my system (which I don't understand).
Compiling the following source:

#include <SDL3/SDL.h>
#include <SDL3/SDL_main.h>
int main(int argc, char *argv[]) {
    Uint32 i;
    for (i = 0; i < 10000000; i++) {
        SDL_SetError("Time me %" SDL_PRIu32, i);
    }
    return 0;
}

and comparing times of SDL3 of current main and this pr branch, I see faster times on this pr branch.
I used "hyperfine" to do the benchmark.
I see 1.3s on current main, and 0.9s on this branch (with.
(This is a SDL built in debug mode)

I tested with SDL3 built in release mode, and I still see the difference: 333 vs 712ms

@icculus
Copy link
Collaborator

icculus commented Sep 29, 2024

I think pass on this for now.

@slouken slouken closed this Sep 29, 2024
@madebr madebr deleted the va_list-for-every-variadic branch September 29, 2024 11:14
@ccawley2011
Copy link
Contributor

For what it's worth, the MorphOS version of SDL2 provides similar functions to the ones in this PR, which suggests it's neccessary on some platforms.

https://github.com/BeWorld2018/SDL/blob/SDL-2.30.7-release/src/SDL_error.c#L63

@slouken
Copy link
Collaborator

slouken commented Oct 13, 2024

I added SDL_SetErrorV() in a567786. All the logging functions can be implemented on top of SDL_LogMessageV(), so I don't think they're necessary.

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.

4 participants