Don’t define functions inline in your public headers
Recently I fixed a bug in librealsense21. The core of the bug was that the developers used a C++ functional cast expression in a C header (C does not have functional cast expressions!), which broke the realsense-rust wrapper we’re developing at work. Worse yet, this was shipped as part of the official 2.42.0 release of librealsense2. Oof.
The fix is pretty clear: use a C-style cast instead. Better yet, follow the advice in Modern C and don’t use casts at all. We can’t always avoid them though, and the compiler can make the whole process annoying. At least, it becomes annoying and difficult to detect such issues when you use a C++ compiler.
Rejecting the premise
I’ve shipped an SDK before myself, and that ended up getting me thinking: I reject the premise of the fix in the first place. This code shouldn’t be broken, and it is frustrating that it is. The real sin here was shipping inline functions in a public header for C code.
Inlining code in C / C++
If that’s confusing, let me expand briefly. Inlining code is a common trick for trying to improve the performance of code by having the compiler copy the implementation of a function and apply it in place (with no additional stack frame). This can improve performance of small code sections, and is critical in optimizing tight loops that centre around small functions. I’d go into extreme depth, but Modern C2 actually has an entire section on this, and it explains it a lot better (look for section 15.1 on inlining functions).
Anyways, the trade-off of inlining functions is that you typically have to place them in the header; alternatively, in
the same source file they’re used in if you’re not exposing the
inline‘d function publicly. Note that this is entirely
about “performance” and not “ergonomics” or “making it easier to code.”
In the scenario where I had to fix the bug above though, there’s a few differences. Rather than directly inlining the
functions, the author of the code chose to make them
static instead. This behaves somewhat similarly to inlining the
code directly but it isn’t exact. In this case, the
static functions in that
- Can end up being instantiated separately in every translation unit (if they’re too big to inline).
- Will make taking a function pointer to any of those functions behave “strangely.” Since that function will be local to each translation unit, taking the pointer of such a function will result in a different pointer for each translation unit.
I suppose the first problem above is a small trade-off as long as the code works the same, but the second one is definitely not something you expect. Particularly, this header is shipped as a C header to librealsense2’s C-API. If you understand C by reading the header, you’ll probably notice the subtle difference, but even still it’s not something that sits at the front of your mind, and may surprise you if you rely on this header in many places and decide to use the function pointer.
Eventually C introduced the
inline keyword to specifically handle these trade-offs when defining functions this way.
Thus, you can get the performance improvement of inlining small functions without the above problems with
functions. However, even if we swap out the
static keyword for
inline we aren’t really solving the underlying
problem at all.
The real issue: inline definitions, not the
The real issue is having those definitions in the header in the first place. By having all that code in your header, you’re shipping a dependency that is basically promising “this code will not break on your compiler.” You don’t know what compiler you’re shipping to; even if your ABI is stable you still have to worry about what standard the language supports, what extensions are available, etc. Inlined definitions are often done best if your code isn’t likely to change ever if at all, but here you can’t be sure.
If you take away anything from this, it should be the importance of not breaking downstream code because of inline definitions. By defining another C file and moving these definitions out of the header, you would probably lose very little, while making it easier to generate FFI bindings in other languages, as well as allow your end users using C to well… use C.
This ended up breaking the FFI generation by bindgen in Rust for us, because the inlined definition changed (and changed
to something that wasn’t standard C). That said, it’s just a bad idea overall. Most of these functions are quite large
already, so chances that the compiler inlines them directly (especially without the
inline) keyword are small.
Moreover, most people doing performance critical code with functions like
rs2_project_color_pixel_to_depth_pixel are unlikely to be banking on the fact that your utility functions are getting
inlined and operating at max efficiency. Just put the definitions in their own file, and only inline the code when you
can clearly benchmark and measure a strong use-case for doing so. Above all else, make sure you understand the language
you’re shipping to when you make that change.
I certainly disagree with the way the code is structured here, but its not an easy problem. It also isn’t something that stems from bad developers, so I can’t accuse Intel of brash incompetence. What likely happened here is that the engineer who made the original change works in C++ for 99% of their work day, and only touches the C headers occasionally when they get tired of compiler warnings or if something significant changes at a lower layer. It’s really easy to make this mistake, even if you do commit to best practices (thanks C!).
At the heart of the issue, working on an SDK is hard and often thankless. I suggest any team that might be shipping C headers take the above advice, and take care to watch that:
- C is not C++, and you should compile your C-API with a C compiler
inlineis an effective tool for performance, not an easy way to avoid committing another
.cfile to your repo.
staticstorage class on functions means something specific, and has downsides when used the same way as
- you should not inline your function definitions in headers you ship publicly for a library! This breaks everyone downstream who is using an FFI or relying on C because they can’t use C++!
I can’t say enough good things about this book. C is complicated in a lot of ways, but if you break things down piece-by-piece you’ll find it’s actually way less bad than many make it out to be. I still love Rust & its type system significantly more, but I’ve worked in C / C++ in the past, so I know from experience where things can start to go awry.
Anyways, I highly suggest giving the book a read, you’ll probably learn something! Jens Gustedt is very well informed and while some opinions (e.g. don’t use casts) are likely to start a flame war, I tend to find myself agreeing with the final assessment. ↩