Unifying naming conventions across libnds

[asiekierka]

Depends on https://github.com/blocksds/sdk/issues/168 - no point in starting this without first tackling that.

One of the many criticisms of libnds over the years has been the rather slapdash nature of its naming scheme. For example, functions currently may use any of the following cases:

• dmaSetParams (camel case, most common)
• BoxTest (Pascal case)
• cothread_create (snake case)
• TMIO_powerupSequence (camel case with a snake prefix)
• setSDcallback (a case)

Some functions (like soundPlayPSG or cardPolledTransfer) are prefixed with a category/namespace; others (like enableSlot1 or writePowerManagement) are not.

Structure types currently may use any of the following cases and styles:

• rtcTimeAndDate (camel case)
• TmioPort (Pascal case)
• TUnpackStruct (Pascal case with a Pascal-style prefix)
• tNDSHeader (Pascal case with a lowercase Pascal-style prefix)
• PERSONAL_DATA (upper-case snake case)
• cothread_info_t (snake case with a _t suffix)
• bg_attribute (snake case)
• s_vramBlock (camel case with a s_ prefix)

Enum types:

• NWRAM_C_SLOT_MASTER (upper-case snake case, most common)
• ObjShape (Pascal case)
• GL_TEXTURE_TYPE_ENUM (Pascal case with an _ENUM suffix)
• PM_Bits (a case)

Private variables and functions exported publicly but used by libnds only internally are also often not prefixed; I think they should be, either with libnds_ or __, and they should not be exposed to the end user. For example, something like int bgInit_call(int layer, BgType type, BgSize size, int mapBase, int tileBase); should be defined inside the bgInit function, so that an user cannot rely on this symbol accidentally in their production code.

The correct naming scheme should be decided and documented as part of the code style, with all functions, types and variables adjusted to match.

Naturally, of course, renaming everything will break the world. Is there something we can do about this? For a previous transition, libnds provided a registers_alt.h file, with legacy register names (slightly different, most notably - not prefixed with REG_).

There is currently a large open source project undergoing a mass rename/refactor - SDL 3. They solved this by:

• adding an SDL_oldnames.h file which contains #defines for all previous names to all new names,
• adding a Python script which can parse it and apply the changes to a project (rename_symbols.py), though just replacing substrings may be too broad for our sometimes non-unique names (like WINDOW),
• providing a script for a semantic C code transformation tool called Coccinelle - its C++ support is an ongoing project.

I believe the best options here for our uses are 1 and 3; option 1 can be realized by creating an nds/oldnames.h file that's only included, for example, for values of BLOCKSDS_STRICT < 20000 (see https://github.com/blocksds/sdk/issues/167 ), and option 3 can probably be generated from such a file.

Note that most other libraries are ineligible - dswifi is maintenance-only, maxmod is already fairly well prefixed, and many other libraries are provided by third parties - they could follow the same roadmap, but we obviously can't force them to.

[profi200]

I feel like i have to explain the reasons why i named my code the way it is.

• TMIO_powerupSequence It's an attempt to mimic C++ namespaces so you know this belongs to the TMIO driver.
• TmioPort Starting with uppercase here to differentiate this from normal variable names. I could also do tmioPort_t or in any of the other styles with _t appended but this is nice and short.
• I also use (almost) only uppercase for macros. I think this is very common. However admittedly i also used this in enums.

Code style is always a matter of preference you will probably get as many opinions as there are devs.

[asiekierka]

Code style is a matter of preference, and I'm not criticizing any particular naming convention here; the goal is to get away from a situation where we have eight naming conventions for structure types, and unify them for the present and future.

(Besides, it should have been my responsibility as the committer in such a situation to unify code styles, not yours.)

[AntonioND]

Private variables and functions exported publicly but used by libnds only internally are also often not prefixed; I think they should be, either with libnds_ or __, and they should not be exposed to the end user. For example, something like int bgInit_call(int layer, BgType type, BgSize size, int mapBase, int tileBase); should be defined inside the bgInit function, so that an user cannot rely on this symbol accidentally in their production code.

https://en.cppreference.com/w/c/keyword

Also, each name that begins with a double underscore _ or an underscore followed by an uppercase letter is reserved

libnds_*** is fine, but __*** is reserved.

Regarding situations like bgInit_call we don't need to use things like functions inside functions, we can just mark everything non-public as static, including private variables.

[asiekierka]

we don't need to use things like functions inside functions, we can just mark everything non-public as static, including private variables.

The idea is that an user should never call bgInit_call (libnds_bgInit_call) directly. By putting the definition inside the function body, it never escapes that block - this means we can define such functions in f.e. static inline functions without making them part of the public API.

[AntonioND]

Yes, but I mean that instead of using C extensions we can just have the functions as regular non-nested functions and mark them as static. I think this helps with clarity because most people won't expect nested functions to be there.

In any case, this is a minor thing, I agree with the rest.

[asiekierka]

https://github.com/blocksds/libnds/pull/106#discussion_r1645153298

Moving away from card/cart to slot1/slot2 should also be discussed as part of this.

[asiekierka]

Yes, but I mean that instead of using C extensions we can just have the functions as regular non-nested functions and mark them as static. I think this helps with clarity because most people won't expect nested functions to be there.

No, that's incorrect. Let me explain based on background.h:

int bgInit_call(int layer, BgType type, BgSize size, int mapBase, int tileBase);

static inline int bgInit(int layer, BgType type, BgSize size, int mapBase, int tileBase)
{
    return bgInit_call(layer, type, size, mapBase, tileBase);
}
// the `bgInit_call` definition still pollutes the user's namespace

versus

static inline int bgInit(int layer, BgType type, BgSize size, int mapBase, int tileBase)
{
    int bgInit_call(int layer, BgType type, BgSize size, int mapBase, int tileBase);
    return bgInit_call(layer, type, size, mapBase, tileBase);
}
// no pollution

You can't use static here, as bgInit_call (or, as it would be renamed to, libnds_bgInit_call) is an external symbol. It's defined in libnds9.a.

[AntonioND]

Ah, okay, I see what you mean. But, to be fair, in that case we should just move the wrapper function to the .c file instead of the .h file, and let the compiler optimize things.

We did something similar in https://github.com/blocksds/libnds/commit/24565646727c3ed05cf6b7897b5524d20279d469 when it become a problem to have the implementation of a function in a header.

[asiekierka]

Ah, okay, I see what you mean. But, to be fair, in that case we should just move the wrapper function to the .c file instead of the .h file, and let the compiler optimize things.

I'm not so sure about that. Remember the compiler won't optimize function calls across translation units unless we get LTO working.