llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.8k stars 11.45k forks source link

Add support for .clang-tidy files to support the --line-filter option #59263

Open daantimmer opened 1 year ago

daantimmer commented 1 year ago

Currently clang-tidy supports the --line-filter option via the command line. However, when clang-tidy is ran from, say, clangd, then it is not possible to provide any command line options for clang-tidy. One of these (important) parameters is the --line-filter option.

I would like to request the --line-filter option to also be available as a .clang-tidy file configuration option. Which results in clangd's usage of clang-tidy to respect the .clang-tidy file and thus the --line-filter option.

llvmbot commented 1 year ago

@llvm/issue-subscribers-clang-tidy

njames93 commented 1 year ago

Moving the flag from the command line to the config likely wouldn't have any effect on clangd as it doesn't handle the options file the same way tidy does natively.

Can I also understand the motivation for this support. hard coding the lines seems like a strange request. The reason for that option at all is so that you can apply clang-tidy just on the diff of the file, I'm not sure I see the use case for it outside of that

daantimmer commented 1 year ago

Can I also understand the motivation for this support. hard coding the lines seems like a strange request. The reason for that option at all is so that you can apply clang-tidy just on the diff of the file, I'm not sure I see the use case for it outside of that

Sure! --line-filter is the only way, it seems, to fully ignore googletest//googlemock macro usage warnings. TEST_F is throwing, with my current clang-tidy config, two warnings. That I can only ignore by either // NOLINT at the end of the line. Which is too much visual noise and just plain annoying. Or by specifying --line-filter=gtest.h (not exactly, forgot the way it is supposed to be written, not at my work PC right now).

Modifying the source of gtest to either ignore or fix the warnings is a no go. Google(test) repo has been declining any and all sorts of pull requests related to fixing or ignoring warnings from 'third party analyzers'. :-(

KindDragon commented 1 year ago

@daantimmer you can try include GTest headers as system include (-isystem). It would probably help you

daantimmer commented 1 year ago

@KindDragon how do you expect that to work? I am using FetchContent from cmake. And it is then up to googletests cmake project to mark those headers as system headers. From my cmakelists I can't really do that...

KindDragon commented 1 year ago

@daantimmer I use sed to patch compile_commands.json after generation

      cat compile_commands.json |
        sed 's# -I../../../thirdparty/googletest/googletest# -isystem../../../thirdparty/googletest/googletest#g' \
          >compile_commands.json.tmp
      mv compile_commands.json.tmp compile_commands.json
daantimmer commented 1 year ago

@KindDragon oof, that sounds like a terrible, non-portable and definitely an unworkable workaround if you ask me.

Not something you can easily deploy to other team members of other projects.

It is, however, a genius solution. (But I won't use it)

KindDragon commented 1 year ago

You can change your CMake script that adds GTest include dir, but it is of course up to you.

ts826848 commented 1 year ago

@KindDragon CMake has the INTERFACE_SYSTEM_INCLUDE_DIRECTORIES property, which you can manually set using set_target_properties. CMake will then use -isystem in the compiler invocation.

KindDragon commented 1 year ago

@ts826848 thanks, but we use GN Build

ts826848 commented 1 year ago

@KindDragon Ah, sorry, pinged the wrong person. My apologies for the mistake!