This is an archive of the discontinued LLVM Phabricator instance.

[libc] Enable llvmlibc clang-tidy checks
ClosedPublic

Authored by PaulkaToast on Mar 21 2020, 5:45 PM.

Details

Summary

Add clang-tidy for llvm-libc source directory.

Example of check in action:

/workspace/llvm-project/libc/src/string/strcpy.cpp:10:1: error: system include stdio.h not allowed [llvmlibc-restrict-system-libc-headers,-warnings-as-errors]
#include <stdio.h>
^~~~~~~~~~~~~~~~~~

Diff Detail

Event Timeline

PaulkaToast created this revision.Mar 21 2020, 5:45 PM
PaulkaToast edited the summary of this revision. (Show Details)
abrachet added inline comments.Mar 21 2020, 6:22 PM
libc/src/.clang-tidy
7

Everything in asm/* should be safe, I imagine.

libc/src/math/round_redirector.cpp
10

Is it possible for value to be something like *redirector* so we don't need to manually add the nolint manually like this? Not that its a big deal as this is the only one.

PaulkaToast marked 2 inline comments as done.Mar 21 2020, 7:39 PM
PaulkaToast added inline comments.
libc/src/.clang-tidy
7

That's what I thought but it has stuff like https://elixir.bootlin.com/linux/latest/source/arch/x86/include/asm/string_32.h

which might be unsafe to include?

libc/src/math/round_redirector.cpp
10

I think it might be better to be on the safe side here and avoid a blanket "allow all includes in redirectors" sort of mechanism. Just to avoid accidentally making a mistake, thoughts?

abrachet accepted this revision.Mar 21 2020, 7:56 PM
abrachet marked 2 inline comments as done.
abrachet added inline comments.
libc/src/.clang-tidy
7

That's not in /usr/include/asm on my machine, and is protected by an #ifdef __KERNEL__ it looks like.

I have no strong preference, we're only using asm/unistd.h I suppose so it makes no big difference either way.

libc/src/math/round_redirector.cpp
10

That's probably better, you're right.

This revision is now accepted and ready to land.Mar 21 2020, 7:56 PM
sivachandra accepted this revision.Mar 21 2020, 10:48 PM

This is OK as a start. But, can we follow up with a study on running clang-tidy as part of our build rules? Like, what are the pros and cons? For the pros, I can think of:

  1. We will not require NOLINT... annotations for redirectors.
  2. Generated headers can be checked.
  3. We force checks as part of our development workflow.

For cons, our builds will now depend on clang and clang-tools-extra. May be there are ways to reduce the "burden".

This revision was automatically updated to reflect the committed changes.