main: SDL_RunApp now explicitly handles NULL argv in all implementations.

[icculus]

It'll usually replace it with { "SDL_app", NULL }, but things like Win32 can query the OS for the original command line arguments.

This allows apps/scripting languages that provide their own entry points to use SDL_RunApp and not have to worry about how to compose an argv array on things like Windows, when SDL was going to do it for them anyhow.

Most things won't experience any change with this commit, including apps that that want extra control but originate in a standard main()-style entry point and can just pass the existing argc/argv through to SDL_RunApp.

Closes #12676.

[castholm]

#12676 (comment)

I went with a smaller approach in #14502 that should accomplish the same thing. I'm going to let that build to make sure I didn't break something before I merge it, but please let me know if it doesn't fit your needs!

To answer your question directly, I believe it should satisfy the use case.

However, my question is, was there anything wrong with #12676 or anything you didn't like about it? In addition to the argv overriding thing, it improves upon the status quo Windows/GDK/Android main handling by reducing allocations and properly cleaning up resources on failure/exit. It also properly documents this behavior and improves general documentation surrounding main by suggesting users to use SDL_RunApp() over SDL_SetMainReady() when possible. In other words, it benefits all users of SDL, not only this slightly niche use case. So why do you think the right call is to effectively redo the work of the existing PR, but... lesser?

[icculus]

However, my question is, was there anything wrong with #12676 or anything you didn't like about it?

Well, I'll tell you, but I want to be clear up front: your work wasn't bad or anything like that. But there were several signals that prompted me to do my own version.

I tend to not look at the individual commits in PRs, because often they are "whoops, I forgot to add this thing," or "corrected something that came up in review," and we'll squash it into a single commit later anyhow, so it's often detrimental to understanding the whole work to look at it commit by commit. This is 100% backwards from how you want revision control to be, but for PRs, I want to understand the entire set of changes we'll be pulling in. So I'm looking at files changed instead of the list of commits.

In this scenario, the first thing I see is we're touching stuff unrelated to the PR itself, and this makes it harder to see the actual thing we're talking about. This is what prompted my comment, "because I suspect some of this should be trimmed out,"

Things like this:

- once in most cases. SDL.h does not include this header automatically.
+ once in most cases. `SDL.h` does not include this header automatically.

Which isn't a bad change to make, and it might have even been in a clearly-delineated commit, but it was background noise vs the actual need here.

There were unrelated patches like this...

- #define SDL_MAIN_AVAILABLE
+ #define SDL_MAIN_AVAILABLE 1

...which was changed for an unrelated bug (and for which this was not the correct fix anyhow). The bug did get fixed due to this (thank you, we didn't know!) but the PR wasn't updated to remove this change.

There was duplicated code added (like this vs this) that I would have rather been unified.

There was this comment that worried me: "I don't think it matters but I'll just quickly note that I swapped the order of GDK_RegisterChangeNotifications() and SDL_SetMainReady() around." It probably doesn't matter, but it adds cognitive load to have to verify that it's a harmless change. We don't have a lot of GDK users at the moment, and if they are shipping console titles they might not update SDL for a long time if ever, so these sort of things risk bugs being introduced that we don't realize for a long time. And, like, I could just as easily cause such a problem myself, of course, but I'll be the one to fix it when I do; most PRs that are fixing a personal need don't get future fixes from the original contributor.

There was a question about changing a public domain licensing comment, which isn't a bad thing to ask, but generally these specific lines are Don't Touch in most projects and we'd have to go request changes to put it back in.

All of these things are innocent in themselves, but as the person that has to decide to adopt these changes and maintain them forever, I--and everyone else with a project on GitHub--has to weigh all these signals and make a decision. The decision may change from hour to hour. Sometimes I merge and tweak little things after in later commits, sometimes I decide the signals aren't a big deal. There are external factors, like this milestone being almost done: do I ask for changes and wait, or do I just run with the original approach on my own and be done today?

So everyone that accepts a PR weighs all these pros and cons, and decides the weight of it. There aren't specific metrics, a lot of it is gut decision. This one landed on "it's better to just make the change that fixes the specific problem" for me, even though the original PR's work was okay. This isn't an ideal system, but that's the system I have.

[castholm]

I'll preface this by saying that I recognize that it's your project and you decide how you to manage it. I have experience with being on both sides of the maintainer/contributor relationship and I completely understand that reviewing and managing exernal contributions is a difficult and time-consuming balancing act that involves a lot of trust. However, some of your justifications here don't feel entirely convincing to me so I would like to address them.

I tend to not look at the individual commits in PRs, because often they are "whoops, I forgot to add this thing," or "corrected something that came up in review," and we'll squash it into a single commit later anyhow, so it's often detrimental to understanding the whole work to look at it commit by commit.

That doesn't seem to be consistently true. I took a look at some of my old merged PRs that had more than one commit, such as this one which had 16, and it wasn't squash-merged, each individual commit was preserved and rebased onto the main branch. This appears to be the case for several other PRs merged in recent times as well.

I obviously understand that random open source contributors often have poor netiquette when it comes to preparing their change sets for review and that commit histories will often be full of "test" "wip" "wip2" "Revert 'wip2'" noise, but I would hope that a reviewer is able to tell "there are good intentions but this history is unworkable" apart from "great, this contributor has put effort into trying to make my life easier" at a quick initial glance. I explicitly structured #12676 such that each commit does one clearly scoped thing and such that individual commits can be dropped before merging or reverted at a later date if they end up causing issues (like the GDK cleanup change you say you are worried about). By squashing you lose out on that ability, as well as the ability to accurately git bisect back to a specific commit that introduced an issue.

There were unrelated patches like this...

Even if you're only looking at the final diff, it should be quite obvious that these changes only affect documentation. I left #define SDL_MAIN_AVAILABLE 1 in because a few lines above you have #define SDL_MAIN_USE_CALLBACKS 1. I realize now as I type this that SDL_MAIN_AVAILABLE is defined without a value in the implementation parts a bit further down so that's an oversight on my part (alternatively, the definitions in the implementation could be updated to 1 for consistency with other feature detection macros like SDL_PLATFORM_*). Regardless, improving documentation while simultaneously making changes to implementations only becomes a problem if you squash everything into one commit.

There was duplicated code added (like this vs this) that I would have rather been unified.

You get the most correct behavior for free, including obscure edge cases, if you store the fallback argv on the stack, but that also means that you can't use a function like your SDL_CheckDefaultArgcArgv(&argc, &argv). Initially when I opened the PR in March I used a SDL_CallMain(argc, argv, mainFunction) but I couldn't find a suitable place for it in the project that didn't feel awkward and without changing the existing file organization by adding new files. I also considered using a macro but it ultimately felt like it would be too much obfuscated magic compared to repeating 5 very simple lines of code.

There was this comment that worried me: "I don't think it matters but I'll just quickly note that I swapped the order of GDK_RegisterChangeNotifications() and SDL_SetMainReady() around."

That comment was mainly me pointing out that that I didn't inadvertently or maliciously change the order of things. Calling SDL_SetMainReady() before main is actually ready, while GDK_RegisterChangeNotifications() could still fail, seemed like an oversight since SDL_SetMainReady() disables certain safeguards. I could have expressed myself more confidently because I did carefully read through the GDK implementation and made sure that none of the global state SDL_SetMainReady() sets up has any effect on GDK_RegisterChangeNotifications().

There was a question about changing a public domain licensing comment, which isn't a bad thing to ask, but generally these specific lines are Don't Touch in most projects and we'd have to go request changes to put it back in.

I completely gutted and swapped out the old implementation to the point where effectively nothing except for the error message boxes remain. Having a comment that implies to the reader that the code they see was written by Sam in 1998 feels like it would be misleading. I'm confident about the correctness of the code, but in the event that there are any problems with it I wouldn't want to misattribute those problems to Sam.

[castholm]

With that said, let's focus on the problems that affect users. There still remain outstanding issues in SDL3 which #12676 would fix:

• The SDL main docs make no mention of SDL_RunApp() as the most portable second best option when a user cannot include SDL_main.h.
• Several main implementations don't call SDL_SetMainReady().
  • Failing to call SDL_SetMainReady() will cause SDL_IsMainThread() to report incorrect values for apps that don't use the video subsystem.
• Android has a !!! FIXME: check for NULL comment regarding a possible null dereference.
• Windows/GDK main perform more allocations/frees than necessary.
• Windows/GDK main will crash on exit, overriding the user's exit code with one indicating failure, if the user mutates argv.
  • Mutating argv is legal since as far back as C89 ("The parameters argc and argv and the strings pointed to by the argv array shall be modifiable by the program, and retain their last-stored values between program startup and program termination."). Since SDL opaquely redefines main it's probably a good idea to retain as many C idiosyncracies as possible.
• GDK main cleanup is spotty and will leak resources when initialization fails.
• GDK main does not call XblCleanupAsync() despite the GDK docs stating that you should.

How would you prefer these be handled? Should I open issues for each of them so that they are first tracked that way? And then submit smaller scoped PRs for each of them?

(Side note: The docs for contributing currently say nothing about how you prefer contributors to structure their PRs with regard to scope, size or keeping histories clean and sensible for reviewers, they mainly only mention code style and tests. If you prefer small PRs which only do one thing, and prefer to adhere to an "always squash-merge" policy, it would be helpful to add that information to the docs so that contributors know what to expect and can make the review process easy for you.)

[icculus]

Okay, well, I'm sure someone will be happy to discuss these items with you.

[slouken]

How would you prefer these be handled? Should I open issues for each of them so that they are first tracked that way? And then submit smaller scoped PRs for each of them?

Yes please.

Thanks!

[andreasgrabher]

Okay, well, I'm sure someone will be happy to discuss these items with you.

I'm not sure this is the right way to treat contributors.