search

google / llvm-premerge-checks

CI system for premerge-testing in LLVM project
Apache License 2.0
41 stars 37 forks source link

Checking a compiler-rt-related patch produces lots of irrelevent clang-tidy diagnostics #231

Open atrosinenko opened 4 years ago

atrosinenko commented 4 years ago

When sending a patch for compiler-rt (specifically, its builtins library), it is annotated it with lots of inline comments that look irrelevant.

An example of such issue: https://reviews.llvm.org/D86221.

To Reproduce Steps to reproduce the behavior:

  1. Submit a refactoring of some LibCall C source
  2. Wait for build to complete
  3. Observe lots of readability-identifier-naming warnings for names like __floatunsisf (these are hardcoded names of compiler support library functions) and single-letter function arguments (such as for fp_t __floatunsisf(su_int a) - this function is one of those converting integers to floats)

Expected behavior 1a. No readability-identifier-naming warnings produced for such libraries 1b. ... or they are produced taking into account the specifics of compiler support libraries

  1. For regular code base parts (C++ code of LLVM and Clang, etc.), readability-identifier-naming warnings are emitted just as usual

Notes Various *_impl.inc files are quite common for builtins sources as they make it possible to instantiate lots of identical LibCalls that differ in sizes of types they operate on. These files are usually not self-contained and as such may produce some errors about undefined type fp_t and so on (these are normally selected based on macroses defined in the topmost C source). While these are not very useful, adding all the *.inc files to ignore lists may be overkill. When fixing such issue for https://reviews.llvm.org/D85031, I implemented a questionable hack like this https://reviews.llvm.org/D85731. Are there some suggestions on how to silence such warnings the right way so the fix will not be invalidated by clang-tidy updates and so on?

Screenshots An example of completely irrelevant warnings: "__floatsisf(si_int a)": invalid case style for function, invalid case style for parameter 'a' An example of viable but most probably either too strict or mis-configured warning: invalid case style for variable 'exponent'

metaflow commented 4 years ago

As I understand warnings were generated for .c files as well (I don't see any .inc D86221 🙈 ). If compiler-rt/lib/builtins/ has own convention than maybe we should add it to exclusions?

atrosinenko commented 4 years ago

I don't see any .inc D86221.

It was D85031 that contain an .inc file and some warnings were already silenced by D85731.

If compiler-rt/lib/builtins/ has own convention than maybe we should add it to exclusions?

It looks like a good idea to me. After all, it should still pass a regular build that will catch obvious mistakes.