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

Added infrastructure for renaming API functions #6871

Merged
merged 2 commits into from
Dec 23, 2022
Merged

Added infrastructure for renaming API functions #6871

merged 2 commits into from
Dec 23, 2022

Conversation

slouken
Copy link
Collaborator

@slouken slouken commented Dec 23, 2022

You can rename APIs using rename.py and all the code and documentation will be updated, and entries will be added to WhatsNew.txt and docs/README-migration.md.
e.g. rename.py SDL_foo.h function SDL_CreateFoo SDL_FooCreate

SDL_oldnames.h is included in the SDL header, and if you define SDL_ENABLE_OLD_NAMES, will redefine the old API functions to call the new ones, and if not, will define them as a symbol letting you what the new API function is.

@slouken slouken added this to the 3.2.0 milestone Dec 23, 2022
@slouken slouken requested a review from a team December 23, 2022 00:50
@slouken slouken marked this pull request as draft December 23, 2022 00:50
@slouken
Copy link
Collaborator Author

slouken commented Dec 23, 2022

This gives us the infrastructure to rename APIs however makes sense, as discussed in #6569.

How do people feel about what was proposed in #6569 (comment)?
I am not talking about a dogmatic requirement, but about a convention that is clear, easy to understand and compatible with IDE tools (code completion, searching text etc.) — it's just a reasonable approach. Eskil Steenberg said something about this in the video entitled. "How I program C" — it is a really good naming that makes working with code much much easier.

The only difference I use from Eskil is a different approach to getters and setters. Eskil uses get and set at the end of the identifiers, while I use them after the subsystem name. This makes it easy to distinguish regular functions from getters and setters. For example, a regular function would be named SDL_WindowMaximize, a getter would be named SDL_WindowGetTitle, and a setter would be named SDL_WindowSetTitle. More examples from the Window API:

Regular functions:

  • SDL_WindowRaise
  • SDL_WindowMinimize
  • SDL_WindowMaximize
  • SDL_WindowCreate
  • SDL_WindowCreateFrom
  • SDL_WindowUpdateSurface
  • SDL_WindowUpdateSurfaceRects ...

Getters (same pattern for setters):

  • SDL_WindowGetSize
  • SDL_WindowGetSizeMinimum
  • SDL_WindowGetSizeMaximum
  • SDL_WindowGetSizeBorders
  • SDL_WindowGetGrab
  • SDL_WindowGetGrabMouse
  • SDL_WindowGetGrabKeyboard ...

See how the words in the identifiers stack up — the beginning fragments are consistent, which is easy to filter, while getters and setters are easy to distinguish from regular functions.

Three variants of getters should also be taken into account, i.e. the prefixes Get, Has and Is, because the latter two are also used in function names currently (and should stay that way) — e.g. SDL_WindowGetTitle, SDL_JoystickIsVirtual, SDL_GameControllerHasAxis.

You can rename APIs using rename.py and all the code and documentation will be updated, and entries will be added to WhatsNew.txt and docs/README-migration.md.
e.g.
rename.py SDL_foo.h function SDL_CreateFoo SDL_FooCreate

SDL_oldnames.h is included in the SDL header, and if you define SDL_ENABLE_OLD_NAMES, will redefine the old API functions to call the new ones, and if not, will define them as a symbol letting you what the new API function is.
@slime73
Copy link
Contributor

slime73 commented Dec 23, 2022

How do people feel about what was proposed in #6569 (comment)?

I don't have a strong opinion about the overall idea (reading documentation plus IDE fuzzy matching for autocomplete lists means the change isn't much of an improvement for me personally I think) - but some of the specifics are a bit weird:

SDL_LockJoysticks => SDL_JoystickLock: the plural is lost here, making it seem like it'd operate on a specific joystick.

SDL_QueueAudio => SDL_AudioQueue: I feel like the new name is a bit ambiguous. Before, it's "queue audio" which is pretty self-descriptive. Now it's "queue something(?) in the audio subsystem" if I'm following along with the general API style. This is minor but still feels like a bit of a regression. Maybe SDL_AudioQueueSampleData or something similar would work well.

SDL_GetNumAudioDevices => SDL_AudioDeviceGetNum, SDL_NumJoysticks => SDL_JoystickNum: I've always been a fan of Count over Num and I feel like Count makes more sense grammatically here - but it's another small nitpick.

SDL_Vulkan_CreateSurface => SDL_SurfaceVulkan_Create, SDL_GL_SwapWindow => SDL_WindowGL_Swap, etc: these feel pretty awkward with their underscore placement and name order compared to the old versions.

SDL_SetWindowsMessageHook => SDL_WindowSetMessageHook: the meaning of this got lost.

@slouken
Copy link
Collaborator Author

slouken commented Dec 23, 2022

I don't think we would want to be militant about this. I agree with all of your examples, I would for example use SDL_JoystickCount() or SDL_JoystickGetCount() and I would leave SDL_SetWindowsMessageHook() alone.

@slouken
Copy link
Collaborator Author

slouken commented Dec 23, 2022

I think the next step once we agree on a general direction is to add a comment for each header with proposed name changes (or maybe a separate issue, for discussion and tracking?)

@slouken
Copy link
Collaborator Author

slouken commented Dec 23, 2022

@furious-programming, would you be willing to go through a few headers and create issues for each of them with a proposed naming change? If people generally like how it's shaping up, I can add commits here implementing those changes.

@icculus
Copy link
Collaborator

icculus commented Dec 23, 2022

SDL_QueueAudio => SDL_AudioQueue

Just as an example, this is where things get tricky, because one of these sounds like a function that does something, and the other sounds like an object you act upon. If I only saw the symbol, I'd assume the first is a function and the second one is a struct.

That's just a quirk of English, not a specific flaw here, but it's definitely something to watch out for.

I think the only thing that is certain is that whoever makes the first pass at this should expect to get some percentage of negative feedback and run under the assumption they might be throwing it away. We're in that awful moment of API redesign where something seems like a good idea until you do the work and find out it isn't, but it has to be done to find out. It's happened to me twice today. :)

It's possible we change all these and say "this is better" or it's possible we change them and say "SDL_CreateWindow was clear and friendly and SDL_WindowCreate feels clinical and I don't know why that is or why this should matter but it does" and then we just fix the SDL_RenderSetX/SDL_SetRenderY mistakes instead and call it a day.

That being said: we are going to rename some things no matter what, so the infrastructure in this PR is good no matter what.

@slouken
Copy link
Collaborator Author

slouken commented Dec 23, 2022

Yup, totally agreed on all of this. I suggested @furious-programming because I think of all of us they’ll do the best job of suggesting helpful changes.

@flowCRANE
Copy link

flowCRANE commented Dec 23, 2022

I don't have a strong opinion about the overall idea (reading documentation plus IDE fuzzy matching for autocomplete lists means the change isn't much of an improvement for me personally I think) - but some of the specifics are a bit weird:

@slime73: the general idea was to unify the naming of library elements by establishing one universal pattern. Making identifier searches easier, whether through IDE tools or searching for text in any text editor (like Vim, Notepad++ etc.), would be an added advantage.

SDL_LockJoysticks => SDL_JoystickLock: the plural is lost here, making it seem like it'd operate on a specific joystick.

The plural must be preserved, so the function should be called SDL_JoysticksLock.

I have several "subsystems" in my game project that distinguish singular from plural. Some units (as Pascal source files) contain the functionality of one piece of object, others are a kind of managers of a collection of a given type of objects. For example:

  • Game_Gamepad — unit containing an object type declaration representing any single gamepad (actually any SDL joystick) and a set of functions to handle it (constructor, destructor, getters, setters and general purpose functions). All functions that operate on a gamepad object have the prefix Game_Gamepad* (singular form).
  • Game_Gamepads — unit containing an internal list of gamepads and a set of functions operating on the pool of available devices. All gamepad pool manipulation functions have the prefix Game_Gamepads* (plural form).

The same for keyboard(s), mouse(s), stick(s) and window(s), and there will be many more in the future.

SDL_QueueAudio => SDL_AudioQueue: I feel like the new name is a bit ambiguous. Before, it's "queue audio" which is pretty self-descriptive. Now it's "queue something(?) in the audio subsystem" if I'm following along with the general API style. This is minor but still feels like a bit of a regression. Maybe SDL_AudioQueueSampleData or something similar would work well.

And here we have a problem, because SDL_AudioQueue can be interpreted in two ways — as mentioned, as a function and as an object type.

In my current project (and any other) I don't have this type of problem because Pascal has a different naming convention than C. In Pascal languages, the data type is always preceded by the letter T, the pointer by P, the interface by I, the argument by A, but the prefix letters are optional, you don't need to use them (they serve only to increase readability). So in this case, I would have three different identifiers, making it clear what the identifier represents:

  • TSDL_AudioQueue — data type representing the queue of the audio subsystem,
  • PSDL_AudioQueue — a pointer to the above data type (PPSDL_AudioQueue as a double pointer),
  • SDL_AudioQueue — the queuing function from the audio subsystem.

In the C language, there is no such thing (and I would rather not suggest using it, because C programmers probably won't love it), so it remains to add something to the end of the SDL_AudioQueue* identifier, so that it is clear what exactly you want to queue.

SDL_GetNumAudioDevices => SDL_AudioDeviceGetNum, SDL_NumJoysticks => SDL_JoystickNum: I've always been a fan of Count over Num and I feel like Count makes more sense grammatically here - but it's another small nitpick.

Pascal also uses Count everywhere, but I like the current Num better because it's shorter and just as clear. A matter of taste. BTW, it should be SDL_AudioDevicesGetNum and SDL_JoysticksGetNum — these are getters, so they should have the Get words.

SDL_Vulkan_CreateSurface => SDL_SurfaceVulkan_Create, SDL_GL_SwapWindow => SDL_WindowGL_Swap, etc: these feel pretty awkward with their underscore placement and name order compared to the old versions.

You're misinterpreting it a bit. The SDL_GL_ and SDL_Vulkan_ prefixes are extraordinary, so they should remain so. These functions, by the convention I suggested, should be named SDL_Vulkan_SurfaceCreate and SDL_GL_WindowSwap.

SDL_SetWindowsMessageHook => SDL_WindowSetMessageHook: the meaning of this got lost.

Not for me, because it is known what subsystem the function comes from — SDL_Window — and what it does — SetMessageHook. What if I added a _? If I did, you would have no problem understanding what the SDL_Window_SetMessageHook function does, just as you now have no problem understanding what the SDL_GL_SwapWindow function does.


What I use myself and what I originally proposed is to use the following pattern:

Prefix         Subsystem    Infix    Postfix

SDL_           Window       Get      Num
SDL_GL_        Mouse        Set      (any other)
SDL_Vulkan_    Keyboard     Has
...            Joystick     Is
               Audio
               Video
               Renderer
               ...

Explanation:

  • Prefix — the prefix is mandatory, specifying which API the identifier comes from. It cannot be changed, it cannot be omitted, must be terminated with a _ character.
  • Subsystem — the name of the subsystem is mandatory, it can consist of one or more words (probably no more than two words), it cannot be omitted. It can be singular or plural, depending on whether it is for a single object operation or for an object pool manager.
  • Infix — optional, intended only for a function, and only to mark it as a getter or setter.
  • Postfix — mandatory, containing any object name or verb, can consist of any number of words. If the function is used to get the number of some objects, it must have the Num prefix (e.g. SDL_JoystickGetNumButtons).

Postfixes should be constructed in such a way that they share initial words. Earlier I gave some such examples:

SDL_WindowCreate
SDL_WindowCreateFrom
SDL_WindowUpdateSurface
SDL_WindowUpdateSurfaceRects

But if you don't want to, then you can ignore it. I use this myself because it makes it easier for me to filter IDs in the completion box. If you don't use completion box, or the completion box can filter in such a way that it splits the identifier into substrings and matches them ignoring the order of the substrings in the identifier, then the above will not be necessary.

Now you need to consider whether you really want to use such a pattern, and if so, you need to check all edge cases (mainly data type names) to make sure that the C naming convention does not cause conflicts and introduce ambiguity. Perhaps the best solution would be to list all the identifiers that exist in the library, group them by subsystem, and then prepare equivalents that match the pattern. In this way, you can quickly check everything and confirm or refute the reasonableness of using the pattern.

@slouken
Copy link
Collaborator Author

slouken commented Dec 23, 2022

Okay, I'll take a first pass at this.

My approach will be to use SDL_ObjectFunction() for functions that take an object as the first parameter, and then rename any functions that are clearly out of line with the others.

@1bsyl
Copy link
Contributor

1bsyl commented Dec 23, 2022

@slouken .. I suggest not to do it manually, but generate all potential renaming like in #6569 . only review all renaming manually to make sure they make sense ...

@1bsyl
Copy link
Contributor

1bsyl commented Dec 23, 2022

Half related to #6869 #6772 . but shouldn't then event be named more like SDL_EVENT_* :
SDL_EVENT_WINDOW_CLOSE for instance, instead of SDL_WINDOWEVENT_CLOSE.

@slouken
Copy link
Collaborator Author

slouken commented Dec 23, 2022

I'm going to go ahead and commit this infrastructure, and we can continue the discussion of how we actually want to change things in #6569

@slouken slouken marked this pull request as ready for review December 23, 2022 19:00
@slouken slouken merged commit b5a9240 into libsdl-org:main Dec 23, 2022
@slouken slouken deleted the api-rename branch December 23, 2022 19:00
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.

5 participants