Skip to content

Conversation

@glandium
Copy link
Contributor

@glandium glandium commented Dec 4, 2024

No description provided.

@glandium
Copy link
Contributor Author

glandium commented Dec 4, 2024

@llvmbot
Copy link
Member

llvmbot commented Dec 4, 2024

@llvm/pr-subscribers-clang-tools-extra

Author: Mike Hommey (glandium)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/118718.diff

1 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/tool/run-clang-tidy.py (+4-1)
diff --git a/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py b/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
index f1b934f7139e94..d4cbcd2f564cbb 100755
--- a/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ b/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -522,7 +522,10 @@ async def main() -> None:
     # Load the database and extract all files.
     with open(os.path.join(build_path, db_path)) as f:
         database = json.load(f)
-    files = {os.path.abspath(os.path.join(e["directory"], e["file"])) for e in database}
+    files = {
+        os.path.normpath(os.path.abspath(os.path.join(e["directory"], e["file"])))
+        for e in database
+    }
     number_files_in_database = len(files)
 
     # Filter source files from compilation database.

@llvmbot
Copy link
Member

llvmbot commented Dec 4, 2024

@llvm/pr-subscribers-clang-tidy

Author: Mike Hommey (glandium)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/118718.diff

1 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/tool/run-clang-tidy.py (+4-1)
diff --git a/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py b/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
index f1b934f7139e94..d4cbcd2f564cbb 100755
--- a/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ b/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -522,7 +522,10 @@ async def main() -> None:
     # Load the database and extract all files.
     with open(os.path.join(build_path, db_path)) as f:
         database = json.load(f)
-    files = {os.path.abspath(os.path.join(e["directory"], e["file"])) for e in database}
+    files = {
+        os.path.normpath(os.path.abspath(os.path.join(e["directory"], e["file"])))
+        for e in database
+    }
     number_files_in_database = len(files)
 
     # Filter source files from compilation database.

@nicovank
Copy link
Contributor

nicovank commented Dec 5, 2024

Is there a reason to bring it back? Is it causing issues?
IIRC this was on purpose because from os.path.abspath:

On most platforms, this is equivalent to calling the function normpath() as follows: normpath(join(os.getcwd(), path)).

@glandium
Copy link
Contributor Author

glandium commented Dec 5, 2024

The change in 315561c causes problems on Windows, and I thought that's what it was, but it turns out it's not. Sorry for the noise. I'm still digging as to what exactly is going wrong on our setup.

@glandium glandium closed this Dec 5, 2024
@glandium
Copy link
Contributor Author

glandium commented Dec 5, 2024

I found what's going on. It kind of worked by chance before in our setup, but that somehow shows an existing flaw in the run-clang-tidy script.
So, our compilation database contains absolute paths with forward slashes. And the paths we pass to the command line are also with forward slashes.
This worked fine because make_absolute had a shortcut that didn't alter the path if it was already absolute. Then file_name_re would match properly.
Now that abspath is always used, the paths in files are all with backslashes (because it's Windows, and path normalization turns the forward slashes into backslashes). But with args.files containing paths with forward slashes, file_name_re doesn't match anymore.
But passing paths with backslashes in paths also doesn't work (and that's not new to 315561c), because of how these paths are used verbatim in file_name_re without escaping. But that's because what's passed in is regexps, so this makes sense.

@nicovank
Copy link
Contributor

nicovank commented Dec 5, 2024

I see, thanks for looking into it! To be honest I think file_name_re could be removed, --source-filter already takes in a regex and filters files based on it, which is more clear. The files arguments could just be taken as-is as a list of file paths (or maybe optionally directories to recurse). This sounds like it would fix this case (and it's closer to what the clang-tidy executable does) but maybe break others.

What do people think? @5chmidti @PiotrZSL @HerrCai0907 @carlosgalvezp?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants