Use of static inline in the public API

[kodebach]

@markus2330 recently voiced concern about the use of static inline ("alias") functions in the public API (e.g in #3730 (comment), #3736 (comment) and #3606). We should discuss how we want to handle these functions moving forward.

IMO, if use carefully, static inline functions can be part of the public API. I will try to explain my reasoning in the following.

The problem with static inline

Normally, if function is part of Elektra's public API this means there is some code written in a .c that is compiled into a library (e.g. libelektra-core). This library exports a symbol that can be used to execute the code. The symbols are declared in header files (e.g. kdb.h) for clients to include into their code.

With a static inline function this is different. The whole function is written in a header file (e.g. kdb.h). The client code does not link to our library and execute the code contain in it. Instead every time the header is included, the compiler copies the whole static inline function into the target file. Depending on various factors, the compiler might then inline the function into it's call sites. Whether this happens is not really important for the public API considerations.

The important difference is that normal functions exist in our .so file and static inline functions exist in client code. If we now change a functions code, this changed version will automatically be used for normal functions. But if a static inline function is changed, the client code has to be recompiled. We call a change where client code has to be recompiled is a "breaking change".

So the problem with static inline function is that a change that might not be breaking otherwise can become a "breaking change" by making a function static inline.

Avoiding breaking changes in static inline functions

The question now is: Can we write static inline function in such a way that non-breaking changes never become breaking changes?

To which I say: Yes, we can, and keyDup is such an example.

libelektra/src/include/kdb.h.in

Lines 210 to 213 in 5702ce0

	static inline Key *keyDup (const Key *source, elektraCopyFlags flags)
	{
	return keyCopy (keyNew ("/", KEY_END), source, flags);
	}

To be clear, we are interested in changes to the function body that normally don't require changes to the call site.

So what could we theoretically change in keyDup:

1. The fact that we call keyNew()
2. The fact that we call keyCopy()
3. The parameters we pass to keyNew()
4. The parameters we pass to keyCopy()

1 and 2 will never change. The definition of keyDup is: "Create a (partial) duplicate of a Key". In other words: "Do a (partial) copy from a Key into a completely new (empty) Key". So we need to call keyNew() to create the new key and we need to call keyCopy to make the copy. The definition also forces us to use these exact arguments to keyNew(), because otherwise we wouldn't get a completely new (empty) Key.

4 is also a non-issue. We already now, that the first parameter for keyCopy() must be a new key, so that can't change. The other parameters to directly passed through from the call-site, so we can't change them either without requiring call-site changes.

There is one important fact we have ignored up until now: What if keyCopy() or keyNew() change in such a way that we must pass different arguments to get the same result (e.g. the first and second arguments of keyCopy() are swapped)? This might indeed require a change inside keyDup() without needing a call-site change. But this is actually also a non-issue. Any such change would by definition be breaking change to keyCopy() or keyNew(), because the change requires a change to the call-site (in this case keyDup()).

So why is a breaking change to keyCopy() or keyNew() not a problem? IMO keyCopy(), keyNew() and keyDup() are all part of the same module (libelektra-core [*]). That means anyone using this module has to check their code and might need to recompile anyway.

In the end the only difference that results from keyDup() being static inline is this: A client that uses keyDup(), but does not use keyCopy() will need to recompile, if there are breaking changes in keyCopy() that can be "hidden" in keyDup()'s implementation (e.g. first to arguments of keyCopy() are swapped). IMO this is not a problem, as long as we write our release notes correctly, so people know when to recompile. Specifically, this means that all breaking changes to keyCopy() or keyNew() must be considered breaking changes to keyDup() as well and this must be stated in the release notes.

Proposal: Rules for static inline functions in the public API

1. Only very short functions. Maybe only one-liners.
2. Don't call functions from a different module. Ideally we'd have 1 header per module, then the rule would be: Don't call functions from a different header.
3. Breaking changes to any function called in a static inline function are also breaking changes for the static inline function. (Must be listed in the release notes).

---

[*] Technically, keyDup() is of course not part of libelektra-core and the way our headers are currently set up, doesn't make it clear where symbols from kdb.h belong. But based on the shared key* prefix, I think my reasoning is valid.

[mpranj]

Do I understand this correctly:

The assumption here is that the client code uses only the static inline portion of the API? Otherwise I assume the symbol versioning would make sure that the client code needs to be recompiled anyway.

In that case we are discussing a scenario that seems very rare to me. Am I overlooking something?

[kodebach]

The assumption here is that the client code uses only the static inline portion of the API? Otherwise I assume the symbol versioning would make sure that the client code needs to be recompiled anyway.

I'm not quite sure what you mean...

I was trying to explain, why it is safe to use static inline in the public API, if we adhere to some rules.

Also the point of symbol versioning is that client code does not need to be recompiled, even if the API changes. The client links against a specific version A of the symbol S. If a breaking change to S happens, the library should provide two versions of S. The old version A (possible with a different implementation) and the new version B. Old clients will still link against version A and won't notice a difference and new clients an use the new version B.

The main thing you have to keep in mind with static inline function is that the body of the static inline function will become part of client code. The rest basically follows from there.

[mpranj]

I'm not quite sure what you mean...

I might be wrong but I was arguing that it is highly unlikely that there would exist useful client code which uses only static inline functions. As soon as the client code links to the shared object, they will need to recompile anyway. This should be very likely unless we make large portions of the API static inline.

I was just looking for confirmation whether the reasoning is correct.

[kodebach]

As soon as the client code links to the shared object, they will need to recompile anyway. This should be very likely unless we make large portions of the API static inline.

I think we might be talking about the same thing, but you may have missed a few "not"s here and there.

---

The point of dynamic linking is that myapp doesn't need to be recompiled when mylib.so is changed (assuming a non-breaking change of course). So if e.g. keyCopy changes it's implementation all applications linking against libelektra-core will pick up this change automatically. But with a static inline function like keyDup the code is copied into the application and is not part of libelektra-core. Therefore a change in keyDup is only picked up, when an application is recompiled. This is what I tried to explain in "The problem with static inline".

In the second part I tried to explain why using static inline is acceptable for some functions. Namely those that don't really have an implementation as such, but are just a shortcut for calling another function of the public API. In such cases it doesn't matter that we cannot change the implementation, because this should never be needed, unless there was a breaking change elsewhere.

The actual proposal is then just some rules for avoiding the problems of static inline functions.

In short, static inline functions don't hide their implementation details. If this is acceptable then using static inline is acceptable. Yet another way to say this is: Writing a static inline is more like putting a piece of code into the docs (which users then copy) than actually writing a public function in a shared object.