[PATCH] D92886: [Sema] Warn about unused functions even when they're inline

Wed Dec 9 10:39:57 PST 2020

aaronpuchert added a comment.

In D92886#2441290 <https://reviews.llvm.org/D92886#2441290>, @Quuxplusone wrote:

> I agree with your reasoning, but in practice I still see a lot of people using `static inline` for functions (especially function templates) in .h files.

That's also what I was seeing, and then I wondered why our warnings don't catch this.

> I'm not sure exactly why — maybe to reduce the burden on the linker, which would otherwise have to make sure all the symbols had the same address, whereas with `static` it doesn't have to worry about that?

I don't think it helps, but perhaps I'm missing something. There are essentially two cases:

- The function is inlined everywhere. Then it's going to be pruned from the object file (because inline functions need to have a definition in all translation units where they're odr-used or something like that), and the linker will never see it.
- The function is not inlined everywhere, so it will be emitted. Then you're right that the linker doesn't have to merge the emitted functions, but that might actually make linking slower, because relocations and so on have to be done for every copy.

My theory is that especially in OOP-heavy code bases programmers develop a habit of thinking in class scope, where `static` has a completely different meaning. At least that's my impression from our code base.

> Have you run this patch over Clang's own codebase, and over libc++? How many positives are there, and do they fall into any thought-provoking patterns?

That's likely a good idea, I will try to run over some code bases.

In D92886#2441410 <https://reviews.llvm.org/D92886#2441410>, @efriedma wrote:

> There's a long history of defining utility functions in headers as "static inline".

Indeed, I wouldn't want to warn about something that never happens anyway.

> Non-static inline functions in C have confusing semantics.

I'm not terribly familiar with C, but judging from C11 6.7.4 it seems that C is a bit more permissive, but then says that certain things are implementation-defined. Whereas in C++ it's forbidden by the one-definition rule that an inline function has another external definition, it's allowed in C but implementation-defined which variant is called.

So if you do non-static inline functions like you'd do them in C++, I think you should be fine. But perhaps I'm missing something, in that case feel free to give me a pointer.

> The warning is designed to be compatible with that reality: it allows people to define "static inline" functions, and still get warnings about other functions that might be unused unintentionally.  I don't think the warning is realistically usable if it doesn't allow "static inline" functions in headers.

Though it does warn about static non-inline functions, and at least for C++ that has the same effect as "static inline" (perhaps even for C). It would be strange if the warning would hinge on a keyword that doesn't actually change anything in that case, and it would suggest to developers that it's a meaningful change.

> Have you tried to see what the practical impact of this change is on real-world codebases?

I'd expect a lot of warnings on our code base at least. We have a bit of a problem with binary sizes, and I think part of that is our usage of internal linkage in header files. (I'm not talking about sure-to-be-inlined functions, but rather templates or functions that shouldn't be in a header.)

Perhaps this is a different issue though, and the proper fix is rather to suppress `unused-*` warnings in headers entirely and have a different warning about using internal linkage in header files.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92886/new/

https://reviews.llvm.org/D92886