llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Jeff Bailey (kaladron) <details> <summary>Changes</summary> The llvm-header-guard check failed to find the project root when working in git worktrees with names other than llvm-project or llvm. When `/llvm-project/` is not in the path, we now walk up the directory tree to find the project root by checking for a `.git` file/directory. The path is then canonicalized to start with `/llvm-project/` before calculating the header guard. Assisted-by: Automated tooling, human reviewed. --- Full diff: https://github.com/llvm/llvm-project/pull/206000.diff 3 Files Affected: - (modified) clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp (+41-1) - (modified) clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.h (+3) - (modified) clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp (+90) ``````````diff diff --git a/clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp b/clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp index ef8b6b1dfb8f7..f34ce4e90505f 100644 --- a/clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp +++ b/clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp @@ -8,6 +8,7 @@ #include "HeaderGuardCheck.h" #include "clang/Tooling/Tooling.h" +#include "llvm/Support/FileSystem.h" #include "llvm/Support/Path.h" namespace clang::tidy::llvm_check { @@ -16,9 +17,48 @@ LLVMHeaderGuardCheck::LLVMHeaderGuardCheck(StringRef Name, ClangTidyContext *Context) : HeaderGuardCheck(Name, Context) {} +// Attempt to find the root of the LLVM project monorepo by walking up the +// directory tree from Filename and looking for a ".git" file/directory. +// This allows us to find the root even when working in git worktrees with +// arbitrary names. +std::string +LLVMHeaderGuardCheck::findLLVMProjectRoot(StringRef Filename) const { + SmallString<256> Path = Filename; + SmallString<256> Parent = llvm::sys::path::parent_path(Path); + while (!Parent.empty() && Parent != Path) { + Path = Parent; + + // Check for .git (file or directory) which indicates the root of the + // git repository or worktree. + SmallString<256> GitPath = Path; + llvm::sys::path::append(GitPath, ".git"); + if (llvm::sys::fs::exists(GitPath)) + return std::string(Path); + + Parent = llvm::sys::path::parent_path(Path); + } + return ""; +} + std::string LLVMHeaderGuardCheck::getHeaderGuard(StringRef Filename, StringRef OldGuard) { - std::string Guard = tooling::getAbsolutePath(Filename); + const std::string AbsolutePath = tooling::getAbsolutePath(Filename); + std::string Guard = AbsolutePath; + + // Check for "/llvm-project/" using a path with normalized slashes to ensure + // Windows paths (which use backslashes) are matched correctly. + const std::string CanonicalPath = + llvm::sys::path::convert_to_slash(AbsolutePath); + if (!StringRef(CanonicalPath).contains("/llvm-project/")) { + const std::string Root = findLLVMProjectRoot(AbsolutePath); + if (!Root.empty()) { + StringRef RelativePath = StringRef(AbsolutePath).substr(Root.size()); + if (!RelativePath.empty() && + llvm::sys::path::is_separator(RelativePath.front())) + RelativePath = RelativePath.drop_front(); + Guard = ("/llvm-project/" + RelativePath).str(); + } + } // When running under Windows, need to convert the path separators from // `\` to `/`. diff --git a/clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.h b/clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.h index cfc920bb85e23..20d8014710531 100644 --- a/clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.h +++ b/clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.h @@ -22,6 +22,9 @@ class LLVMHeaderGuardCheck : public utils::HeaderGuardCheck { bool shouldSuggestEndifComment(StringRef Filename) override { return false; } std::string getHeaderGuard(StringRef Filename, StringRef OldGuard) override; + +protected: + virtual std::string findLLVMProjectRoot(StringRef Filename) const; }; } // namespace clang::tidy::llvm_check diff --git a/clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp b/clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp index e1338c6ef1d5f..9638a116ec904 100644 --- a/clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp +++ b/clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp @@ -60,6 +60,24 @@ runHeaderGuardCheckWithEndif(StringRef Code, const Twine &Filename, std::optional<StringRef> ExpectedWarning) { return runCheck<WithEndifComment>(Code, Filename, std::move(ExpectedWarning)); } +static std::string GlobalMockRoot; + +struct MockLLVMHeaderGuardCheck : public LLVMHeaderGuardCheck { + MockLLVMHeaderGuardCheck(StringRef Name, ClangTidyContext *Context) + : LLVMHeaderGuardCheck(Name, Context) {} + std::string findLLVMProjectRoot(StringRef Filename) const override { + return GlobalMockRoot; + } +}; + +static std::string +runMockHeaderGuardCheck(StringRef Code, const Twine &Filename, + std::optional<StringRef> ExpectedWarning, + std::string MockRoot) { + GlobalMockRoot = MockRoot; + return runCheck<MockLLVMHeaderGuardCheck>(Code, Filename, + std::move(ExpectedWarning)); +} } // namespace TEST(LLVMHeaderGuardCheckTest, FixHeaderGuards) { @@ -297,6 +315,78 @@ TEST(LLVMHeaderGuardCheckTest, FixHeaderGuards) { "", "\\\\?\\C:\\llvm-project\\clang-tools-extra\\clangd\\foo.h", StringRef("header is missing header guard"))); #endif + + // Test path outside any git repo (real root detection should fail and not + // hang). + EXPECT_EQ("#ifndef NON_EXISTENT_DIRECTORY_123_X_H\n" + "#define NON_EXISTENT_DIRECTORY_123_X_H\n" + "\n" + "\n" + "#endif\n", + runHeaderGuardCheck("", "/non_existent_directory_123/x.h", + StringRef("header is missing header guard"))); + + // Test single-component absolute path (loop boundary, doesn't hang). + EXPECT_EQ("#ifndef X_H\n" + "#define X_H\n" + "\n" + "\n" + "#endif\n", + runHeaderGuardCheck("", "/x.h", + StringRef("header is missing header guard"))); +} + +TEST(LLVMHeaderGuardCheckTest, MockedProjectRoot) { + EXPECT_EQ("#ifndef LLVM_LIBC_SRC_STRING_STRLEN_H\n" + "#define LLVM_LIBC_SRC_STRING_STRLEN_H\n" + "\n" + "\n" + "#endif\n", + runMockHeaderGuardCheck( + "", "/my/mock/root/libc/src/string/strlen.h", + StringRef("header is missing header guard"), "/my/mock/root")); + + EXPECT_EQ( + "#ifndef SOME_OTHER_PATH_LIBC_SRC_STRING_STRLEN_H\n" + "#define SOME_OTHER_PATH_LIBC_SRC_STRING_STRLEN_H\n" + "\n" + "\n" + "#endif\n", + runMockHeaderGuardCheck("", "/some/other/path/libc/src/string/strlen.h", + StringRef("header is missing header guard"), "")); + + EXPECT_EQ("#ifndef LLVM_LIBC_SRC_STRING_STRLEN_H\n" + "#define LLVM_LIBC_SRC_STRING_STRLEN_H\n" + "\n" + "\n" + "#endif\n", + runMockHeaderGuardCheck( + "", "/path/to/llvm-project/libc/src/string/strlen.h", + StringRef("header is missing header guard"), + "/different/mock/root")); + + // Test when root has a trailing slash. + EXPECT_EQ("#ifndef LLVM_LIBC_SRC_STRING_STRLEN_H\n" + "#define LLVM_LIBC_SRC_STRING_STRLEN_H\n" + "\n" + "\n" + "#endif\n", + runMockHeaderGuardCheck( + "", "/my/mock/root/libc/src/string/strlen.h", + StringRef("header is missing header guard"), "/my/mock/root/")); + +#ifdef WIN32 + // Test Windows-style paths through the new code path (mocked root). + EXPECT_EQ("#ifndef LLVM_LIBC_SRC_STRING_STRLEN_H\n" + "#define LLVM_LIBC_SRC_STRING_STRLEN_H\n" + "\n" + "\n" + "#endif\n", + runMockHeaderGuardCheck( + "", "C:\\my\\mock\\root\\libc\\src\\string\\strlen.h", + StringRef("header is missing header guard"), + "C:\\my\\mock\\root")); +#endif } TEST(IncludeOrderCheck, GTestHeaders) { `````````` </details> https://github.com/llvm/llvm-project/pull/206000 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
