[PATCH] D124447: [clang-tidy] Add infrastructure support for running on project-level information

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

The idea looks great in general, I didn't get a chance to look at all the details but this also creates some concerns for integrations of clang-tidy checks in other environments (like clangd or tidy running in distributed systems) as the workflow actually needs to be run twice and be able to write to disk.

So I'd actually ask for changing the way the outputting is performed. It'd be better to have an interface where checks can perform this finding reporting operation, that way clangd and other means of running clang-tidy can implement that interface as they see fit (put everything into a database, collect in memory, etc.) and clang-tidy binary by default can implement writing to disk (through VFS rather than physical FS operations if possible).
In addition to that it'd be nice to have a flag on checks to indicate whether they can work in a certain phase, and make sure instantiation only happens for checks that can run at a certain phase. This ensures checks can have assumptions about the phase they're being run at and also drivers can assume checks won't perform invalid operations. That way we should get to reduce complexity on both sides. We can by default say that checks work in diagnose phase and doesn't work in other phases, then relevant checks can override these bits.

As a more general comment to the overall approach, the "compact" phase probably doesn't require any tidy specific infrastructure. Only pieces that can benefit re-use is outputting edits, but this is already common infra across all clang-tools. So another direction overall here could be to actually change the way clang-tidy outputs findings into two forms:

- a machine readable format, that can later on be used by other tools to perform more analysis
- a human readable format, the usual thing today.

Have we considered this? Any reason for not going down this path?



================
Comment at: clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:98
+  OS << Context->getGlobalOptions().MultipassDirectory << get_separator()
+     << CheckName << '.' << filename(CurrentFile) << '.'
+     << hash_value(CurrentFile) << ".yaml";
----------------
filename and contents might not be enough here. as clang-tidy is usually run over a compilation database and there might be multiple entries for the same file with different compile flags. i think we also want to include a hash of the flags in the filename.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124447