Hi Théo, First of all, apologies that it has taken me so long to review this! On Thu, Jul 04, 2024 at 11:28:21AM +0200, Théo Lebrun wrote: > Add file filtering feature. We take zero or more filters at the end as > positional arguments. If none are given, the default behavior is kept > and we run the tool on all files in the datastore. Else, files must > match one or more filter to be analysed. > > The below command runs clang-tidy on drivers/clk/clk.c and all C files > inside drivers/reset/. > > ./scripts/clang-tools/run-clang-tools.py clang-tidy \ > compile_commands.json \ > 'drivers/clk/clk.c' 'drivers/reset/*' > > The Python fnmatch builtin module is used. Matching is case-insensitive. > See its documentation for allowed syntax: > https://docs.python.org/3/library/fnmatch.html > > Signed-off-by: Théo Lebrun <theo.lebrun@xxxxxxxxxxx> > --- > Currently, all files in the datastore are analysed. This is not > practical for grabbing errors in a subsystem, or relative to a patch > series. Add a file filtering feature with wildcard support. Sure, I think this is totally reasonable. In fact, I think some of this could be added to the commit message as further existence for this feature. The change itself looks good to me for the most part, I have some questions below just for my own understanding. Reviewed-by: Nathan Chancellor <nathan@xxxxxxxxxx> One further question/comment now: Have you considered a way to integrate this into Kbuild with the clang-tidy and clang-analyzer commands? I don't think it is strictly necessary for the acceptance of this patch but it might be nice to have some variable that users could provide to do this with their regular make command + the clang-tidy target? Not sure if Masahiro has further thoughts on that. > Have a nice day, > Théo > --- > scripts/clang-tools/run-clang-tools.py | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/scripts/clang-tools/run-clang-tools.py b/scripts/clang-tools/run-clang-tools.py > index f31ffd09e1ea..b0b3a9c8cdec 100755 > --- a/scripts/clang-tools/run-clang-tools.py > +++ b/scripts/clang-tools/run-clang-tools.py > @@ -10,6 +10,7 @@ compile_commands.json. > """ > > import argparse > +import fnmatch > import json > import multiprocessing > import subprocess > @@ -32,6 +33,8 @@ def parse_arguments(): > help=type_help) > path_help = "Path to the compilation database to parse" > parser.add_argument("path", type=str, help=path_help) > + file_filter_help = "Optional Unix shell-style wildcard file filters" > + parser.add_argument("file_filter", type=str, nargs="*", help=file_filter_help) > > checks_help = "Checks to pass to the analysis" > parser.add_argument("-checks", type=str, default=None, help=checks_help) > @@ -48,6 +51,22 @@ def init(l, a): > args = a > > > +def filter_entries(datastore, filters): > + for entry in datastore: > + if filters == []: > + yield entry > + continue > + > + assert entry['file'].startswith(entry['directory']) What is the purpose of this assertion? Will it cause AssertionError under normal circumstances? > + # filepath is relative to the directory, to avoid matching on the absolute path > + filepath = entry['file'][len(entry['directory']):].lstrip('/') > + > + for pattern in filters: > + if fnmatch.fnmatch(filepath, pattern): > + yield entry > + break > + > + > def run_analysis(entry): > # Disable all checks, then re-enable the ones we want > global args > @@ -87,6 +106,7 @@ def main(): > # Read JSON data into the datastore variable > with open(args.path, "r") as f: > datastore = json.load(f) > + datastore = filter_entries(datastore, args.file_filter) > pool.map(run_analysis, datastore) > except BrokenPipeError: > # Python flushes standard streams on exit; redirect remaining output > > --- > base-commit: 22a40d14b572deb80c0648557f4bd502d7e83826 > change-id: 20240704-clang-tidy-filter-f470377cb2b4 > > Best regards, > -- > Théo Lebrun <theo.lebrun@xxxxxxxxxxx> >
