kcat / alure

Alure is a utility library for OpenAL, providing a C++ API and managing common tasks that include file loading, caching, and streaming
zlib License
70 stars 20 forks source link

static inline functions that are in source files? #16

Closed Cazadorro closed 4 years ago

Cazadorro commented 6 years ago

I'm confused as to why there are static inline functions for example in auxeffectslot.cpp or context.cpp. I don't understand what this accomplishes. Is this only used as a guarantee that these functions can only be used within the given source file? If so I'm confused as to why such functions weren't just defined in the source file, I've never seen both static and inline functions in used in a cpp file in C++ before. I don't see the advantage of having "multiple definition" of in this specific circumstance, and "only this translation unit", it appears that you would never have multiple definition, and it would always be defined for that translation unit if you just defined a function?

I also saw this in effect.cpp:

namespace {

template<typename T>
inline T clamp(const T& val, const T& min, const T& max)
{ return std::min<T>(std::max<T>(val, min), max); }

} // namespace

which really made me scratch my head. why is this inlined, why is it in an empty namespace. It looks like you just wanted clamp for this file, even if using C++17 and didn't want to overwrite it any where else? I guess its inlined because you want to have multiple definitions of the same signature? wouldn't it just be better to provide a header to that defines this if you don't have c++17 or just uses c++17 if you have it?

kcat commented 6 years ago

I'm confused as to why there are static inline functions for example in auxeffectslot.cpp or context.cpp. I don't understand what this accomplishes.

static means the function is only accessible to the current source file, and inline tells the compiler I'd prefer the function to be inlined if possible without relying on its heuristics to automatically inline it. So it's for creating small local functions that should be inlined into the caller and other source files shouldn't see or don't need.

The proper "C++ way" is probably to use an anonymous namespace instead of making it static, but it's the same effect.

I also saw this in effect.cpp: ... which really made me scratch my head. why is this inlined, why is it in an empty namespace.

It's inlined for the same reason as above, to avoid the overhead of calling it as a function. Especially with smaller functions, the amount of work necessary to call it can be about as much as the function itself, so inlining the code makes such cases faster. It's in an anonymous namespace to avoid clashes in case another source also defines a clamp function (if it ends up used in multiple sources, it would be beneficial to move it into a header).

Cazadorro commented 6 years ago

inline tells the compiler I'd prefer the function to be inlined if possible without relying on its heuristics to automatically inline it

Note the primary reason for modern libraries use inline is not to hint the compiler, but to allow multiple definitions in translation units, as the compiler doesn't have to care about your hints and will likely inline things better anyway. in other words:

"function marked inline can be defined in more than one translation unit without violating the One Definition Rule (ODR)". It seems there is some consensus now to not use it for hinting at all for modern compilers.

https://softwareengineering.stackexchange.com/questions/35432/inline-functions-in-c-whats-the-point

https://stackoverflow.com/questions/29796264/is-there-still-a-use-for-inline?noredirect=1&lq=1

https://stackoverflow.com/questions/1759300/when-should-i-write-the-keyword-inline-for-a-function-method

kcat commented 6 years ago

Note the primary reason for modern libraries use inline is not to hint the compiler, but to allow multiple definitions in translation units, as the compiler doesn't have to care about your hints and will likely inline things better anyway.

I still prefer to be explicit that I want a function inlined, rather than hoping the compiler has the correct heuristics enabled to automatically see it could be inlined. It also helps me know what the compiler's doing, as it will warn when a function marked inline isn't inlined (and also why it isn't, either being too large or too unlikely to be called), while it won't warn that an unmarked function I think should be inlined isn't.

Cazadorro commented 6 years ago

It also helps me know what the compiler's doing, as it will warn when a function marked inline isn't inlined

Huh, that is pretty significant, I didn't even think to use that as a sort of performance debugging utility.

Cazadorro commented 4 years ago

issues solved in comments