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

Improving the formatting of header files #9405

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aganm
Copy link

@aganm aganm commented Mar 31, 2024

In light of "Another GPU API (#9312)", the different formatting made me remember that I
always had an issue with SDL's function's formatting, specifically in header files.

The way function signatures are formatted in header files make it difficult to read in a thin split window
because function signatures oftentimes go far outside the 80 column limit.

See this SDL example, where all the documentation is nicely layed out to fit in a 80 column window, but not the signature:

/**
 * Get the scale mode used for texture scale operations.
 *
 * \param texture the texture to query.
 * \param scaleMode a pointer filled in with the current scale mode.
 * \returns 0 on success or a negative error code on failure; call
 *          SDL_GetError() for more information.
 *
 * \since This function is available since SDL 3.0.0.
 *
 * \sa SDL_SetTextureScaleMode
 */
extern DECLSPEC int SDLCALL SDL_GetTextureScaleMode(SDL_Texture *texture, SDL_ScaleMode *scaleMode);

The formatting of function signatures in "Another GPU API (#9312)" look like this:

extern DECLSPEC void SDLCALL SDL_GpuDownloadFromTexture(
	SDL_GpuDevice *device,
	SDL_GpuTextureRegion *textureRegion,
	SDL_GpuTransferBuffer *transferBuffer,
	SDL_GpuBufferImageCopy *copyParams,
	SDL_GpuTransferOptions transferOption
);

Personnally, putting the closing parenthese on a new line at the end is wasted whitespace and I would keep it on the last parameter. The SDL example would look like this with the new formatting, and now it reads super nice in a 80 column window:

/**
 * Get the scale mode used for texture scale operations.
 *
 * \param texture the texture to query.
 * \param scaleMode a pointer filled in with the current scale mode.
 * \returns 0 on success or a negative error code on failure; call
 *          SDL_GetError() for more information.
 *
 * \since This function is available since SDL 3.0.0.
 *
 * \sa SDL_SetTextureScaleMode
 */
extern DECLSPEC int SDLCALL SDL_GetTextureScaleMode(
    SDL_Texture *texture,
    SDL_ScaleMode *scaleMode);

What I propose is this: The formatting of function signatures in header files must have their parameter list layed out vertically below the function name, unless parameters follow some kind of logical grouping, then these can go on the same line together. Logical grouping is not set in stone, pick whatever is most readable. Like in this example:

extern DECLSPEC int SDLCALL SDL_UpdateYUVTexture(
    SDL_Texture *texture,
    const SDL_Rect *rect,
    const Uint8 *Yplane, int Ypitch,
    const Uint8 *Uplane, int Upitch,
    const Uint8 *Vplane, int Vpitch);

This PR contains the reformatting for SDL_render.h only, it merely took a few minutes to redo the formatting.

Tell me what you think, and I shall proceed with the rest of the headers.

@icculus
Copy link
Collaborator

icculus commented Mar 31, 2024

I haven't looked at this yet, but if we do this, we have to make sure gendynapi.py and wikibridge.pl can handle multiline function declarations.

@slouken
Copy link
Collaborator

slouken commented Apr 1, 2024

They can. I've actually been moving the other way, since in SDL2 we had a mix of declarations split to 80 columns and all on a single line.

Whichever way we decide to go, the wiki bridge should be updated to automate this for us. :)

@aganm aganm force-pushed the formatting branch 2 times, most recently from b5d3f92 to 634db23 Compare June 1, 2024 12:10
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.

None yet

3 participants