Refactor CheckIncludesStrictExceptions API This refactoring prepares for disabling check_includes_strict optimizations when exemptions are present: - `empty()` allows a fast path when no exceptions are configured, enabling optimizations without checking individual targets. - `HasGlobalException()` and `HasLocalException()` allow querying global and target-specific rules independently, which is needed to inspect whether a target or its private dependencies are subject to exemptions. - Rename `AddException` to `AddLocalException` to clearly differentiate from `AddGlobalException`. - Rename `CanInclude` to `HasException`. - Update callers and unittests accordingly. Bug: 556959580 Change-Id: I62c5ef3ab553e5fdeb88b3395e5c370e6a6a6964 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27620 Commit-Queue: Matt Stark <msta@google.com> Reviewed-by: Takuto Ikuta <tikuta@google.com>
diff --git a/src/gn/check_includes_strict_exceptions.cc b/src/gn/check_includes_strict_exceptions.cc index a680838..a5eb6e6 100644 --- a/src/gn/check_includes_strict_exceptions.cc +++ b/src/gn/check_includes_strict_exceptions.cc
@@ -9,11 +9,15 @@ #include "gn/scope.h" #include "gn/value.h" -bool CheckIncludesStrictExceptions::CanInclude(const Label& from, - const Label& to) const { - if (LabelPattern::VectorMatches(global_, from)) - return true; +bool CheckIncludesStrictExceptions::HasGlobalException( + const Label& from) const { + return LabelPattern::VectorMatches(global_, from); +} +bool CheckIncludesStrictExceptions::HasLocalException(const Label& from, + const Label& to) const { + if (specific_targets_.empty()) + return false; auto it = specific_targets_.find(to.GetWithNoToolchain()); if (it != specific_targets_.end()) return LabelPattern::VectorMatches(it->second, from); @@ -21,6 +25,11 @@ return false; } +bool CheckIncludesStrictExceptions::HasException(const Label& from, + const Label& to) const { + return HasGlobalException(from) || HasLocalException(from, to); +} + Result<CheckIncludesStrictExceptions> CheckIncludesStrictExceptions::FromValue( const Value& value, const SourceDir& current_dir, @@ -63,7 +72,7 @@ if (!included->string_value().starts_with("//")) return Err(*included, "Target label must be absolute (starting with '//')."); - exceptions.AddException(from, to); + exceptions.AddLocalException(from, to); } else { exceptions.AddGlobalException(from); }
diff --git a/src/gn/check_includes_strict_exceptions.h b/src/gn/check_includes_strict_exceptions.h index c1e4c53..2c616ed 100644 --- a/src/gn/check_includes_strict_exceptions.h +++ b/src/gn/check_includes_strict_exceptions.h
@@ -5,7 +5,6 @@ #ifndef TOOLS_GN_CHECK_INCLUDES_STRICT_EXCEPTIONS_H_ #define TOOLS_GN_CHECK_INCLUDES_STRICT_EXCEPTIONS_H_ -#include <string> #include <string_view> #include <unordered_map> #include <vector> @@ -28,12 +27,21 @@ std::string_view source_root); void AddGlobalException(const LabelPattern& from) { global_.push_back(from); } - void AddException(const LabelPattern& from, const Label& to) { + void AddLocalException(const LabelPattern& from, const Label& to) { specific_targets_[to.GetWithNoToolchain()].push_back(from); } - // Checks whether `from` is allowed to include headers from `to`. - bool CanInclude(const Label& from, const Label& to) const; + // Checks whether `(from, to)` matches any local or global exception. + bool HasException(const Label& from, const Label& to) const; + + // Checks whether `from` matches any global exception rule. + bool HasGlobalException(const Label& from) const; + + // Checks whether `(from, to)` matches any local exception rule. + bool HasLocalException(const Label& from, const Label& to) const; + + // Returns true if there are no exception rules. + bool empty() const { return global_.empty() && specific_targets_.empty(); } private: std::vector<LabelPattern> global_;
diff --git a/src/gn/check_includes_strict_exceptions_unittest.cc b/src/gn/check_includes_strict_exceptions_unittest.cc index 7794eb7..29c249b 100644 --- a/src/gn/check_includes_strict_exceptions_unittest.cc +++ b/src/gn/check_includes_strict_exceptions_unittest.cc
@@ -44,9 +44,9 @@ specific_list.list_value().push_back(make_rule("//:includer", "//:included")); auto specific_exceptions = parse(specific_list); ASSERT_SUCCESS(specific_exceptions); - EXPECT_TRUE(specific_exceptions->CanInclude( + EXPECT_TRUE(specific_exceptions->HasException( Label(SourceDir("//"), "includer"), Label(SourceDir("//"), "included"))); - EXPECT_FALSE(specific_exceptions->CanInclude( + EXPECT_FALSE(specific_exceptions->HasException( Label(SourceDir("//"), "includer"), Label(SourceDir("//"), "other"))); // Global exception (included omitted). @@ -55,28 +55,28 @@ auto global_exceptions = parse(global_list); ASSERT_SUCCESS(global_exceptions); EXPECT_TRUE( - global_exceptions->CanInclude(Label(SourceDir("//"), "includer"), - Label(SourceDir("//any/"), "target"))); + global_exceptions->HasException(Label(SourceDir("//"), "includer"), + Label(SourceDir("//any/"), "target"))); EXPECT_FALSE( - global_exceptions->CanInclude(Label(SourceDir("//other/"), "target"), - Label(SourceDir("//any/"), "target"))); + global_exceptions->HasException(Label(SourceDir("//other/"), "target"), + Label(SourceDir("//any/"), "target"))); } -TEST(CheckIncludesStrictExceptionsTest, CanInclude) { +TEST(CheckIncludesStrictExceptionsTest, HasException) { CheckIncludesStrictExceptions exceptions; - exceptions.AddException( + exceptions.AddLocalException( LabelPattern(LabelPattern::RECURSIVE_DIRECTORY, SourceDir("//includer_recursive/"), "", Label()), Label(SourceDir("//included/"), "target")); - EXPECT_TRUE(exceptions.CanInclude( + EXPECT_TRUE(exceptions.HasException( Label(SourceDir("//includer_recursive/sub/"), "target"), Label(SourceDir("//included/"), "target"))); EXPECT_FALSE( - exceptions.CanInclude(Label(SourceDir("//other_dir/"), "target"), - Label(SourceDir("//included/"), "target"))); - EXPECT_FALSE(exceptions.CanInclude( + exceptions.HasException(Label(SourceDir("//other_dir/"), "target"), + Label(SourceDir("//included/"), "target"))); + EXPECT_FALSE(exceptions.HasException( Label(SourceDir("//includer_recursive/sub/"), "target"), Label(SourceDir("//included/"), "other_target"))); @@ -84,9 +84,9 @@ LabelPattern::DIRECTORY, SourceDir("//includer_global/"), "", Label())); EXPECT_TRUE( - exceptions.CanInclude(Label(SourceDir("//includer_global/"), "foo"), - Label(SourceDir("//any_dir/"), "any_target"))); + exceptions.HasException(Label(SourceDir("//includer_global/"), "foo"), + Label(SourceDir("//any_dir/"), "any_target"))); EXPECT_FALSE( - exceptions.CanInclude(Label(SourceDir("//other_dir/"), "foo"), - Label(SourceDir("//any_dir/"), "any_target"))); + exceptions.HasException(Label(SourceDir("//other_dir/"), "foo"), + Label(SourceDir("//any_dir/"), "any_target"))); }
diff --git a/src/gn/header_checker.cc b/src/gn/header_checker.cc index 7f31064..69c2c95 100644 --- a/src/gn/header_checker.cc +++ b/src/gn/header_checker.cc
@@ -681,7 +681,7 @@ // simpler this way, and it's pretty rare anyway. bool strict_exempt = false; for (const auto& target : targets) { - if (build_settings_->check_includes_strict_exceptions().CanInclude( + if (build_settings_->check_includes_strict_exceptions().HasException( from_target->label(), target.target->label())) { strict_exempt = true; break;
diff --git a/src/gn/header_checker_unittest.cc b/src/gn/header_checker_unittest.cc index 2d41462..7ce47af 100644 --- a/src/gn/header_checker_unittest.cc +++ b/src/gn/header_checker_unittest.cc
@@ -718,9 +718,9 @@ // Set exceptions on build_settings: CheckIncludesStrictExceptions exceptions; LabelPattern a_dir(LabelPattern::DIRECTORY, SourceDir("//a/"), "", Label()); - exceptions.AddException(a_dir, b_.label()); - exceptions.AddException(a_dir, a_.label()); - exceptions.AddException(a_dir, c_.label()); + exceptions.AddLocalException(a_dir, b_.label()); + exceptions.AddLocalException(a_dir, a_.label()); + exceptions.AddLocalException(a_dir, c_.label()); setup_.build_settings()->set_check_includes_strict_exceptions( std::move(exceptions));