From 56dbfbe9cae76cf18aabf9c14f099dc9609487f3 Mon Sep 17 00:00:00 2001 From: Kim Grasman Date: Sun, 10 Mar 2019 16:45:14 +0100 Subject: [PATCH] Remove Google-specific handling of third-party This is part of Google's original open-sourcing of IWYU. They keep third-party libraries in a directory called, well, 'third-party', so there was lots of special casing for code in that directory. Remove that code, unit tests covering it and explicit mappings. Keep one special case for allowing include cycles for files with 'internal/' in the name, to avoid breaking the include_cycle test case. Not sure what to do about that longer term, but I didn't want to remove the test case right now. --- iwyu.gcc.imp | 1 - iwyu_include_picker.cc | 67 +-------- iwyu_include_picker.h | 4 - iwyu_path_util.cc | 16 -- iwyu_path_util.h | 4 - iwyu_preprocessor.cc | 3 +- more_tests/iwyu_include_picker_test.cc | 199 ------------------------- third_party.imp | 25 ---- 8 files changed, 4 insertions(+), 315 deletions(-) delete mode 100644 third_party.imp diff --git a/iwyu.gcc.imp b/iwyu.gcc.imp index 2889e77..8aa4b43 100644 --- a/iwyu.gcc.imp +++ b/iwyu.gcc.imp @@ -4,5 +4,4 @@ { ref: gcc.symbols.imp }, { ref: gcc.stl.headers.imp }, { ref: stl.c.headers.imp }, - { ref: third_party.imp } ] diff --git a/iwyu_include_picker.cc b/iwyu_include_picker.cc index 3612c30..145fd0c 100644 --- a/iwyu_include_picker.cc +++ b/iwyu_include_picker.cc @@ -913,14 +913,9 @@ void MakeNodeTransitive(IncludePicker::IncludeMap* filename_map, // If we've already calculated this node's transitive closure, we're done. const TransitiveStatus status = (*seen_nodes)[key]; if (status == kCalculating) { // means there's a cycle in the mapping - // third-party code sometimes has #include cycles (*cough* boost - // *cough*). Because we add many implicit third-party mappings, - // we may add a cycle without meaning to. The best we can do is - // to ignore the mapping that causes the cycle. Same with code - // in /internal/. We could CHECK-fail in such a case, but it's - // probably better to just keep going. - if (StartsWith(key, "\"third_party/") || - key.find("internal/") != string::npos) { + // TODO: Reconsider cycle handling; the include_cycle test fails without + // this special-casing, but it seems we should handle this more generally. + if (key.find("internal/") != string::npos) { VERRS(4) << "Ignoring a cyclical mapping involving " << key << "\n"; return; } @@ -1243,58 +1238,6 @@ void IncludePicker::ExpandRegexes() { } } -// We treat third-party code specially, since it's difficult to add -// iwyu pragmas to code we don't own. Basically, what we do is trust -// the code authors when it comes to third-party code: if they -// #include x.h to get symbols from y.h, then assume that's how the -// third-party authors wanted it. This boils down to the following -// rules: -// 1) If there's already a mapping for third_party/y.h, do not -// add any implicit maps for it. -// 2) if not_third_party/x.{h,cc} #includes third_party/y.h, -// assume y.h is supposed to be included directly, and do not -// add any implicit maps for it. -// 3) Otherwise, if third_party/x.h #includes third_party/y.h, -// add a mapping from y.h to x.h. Unless y.h already has -// a hard-coded visibility set, make y.h private. This -// means iwyu will never suggest adding y.h. -void IncludePicker::AddImplicitThirdPartyMappings() { - set third_party_headers_with_explicit_mappings; - for (const IncludeMap::value_type& item : filepath_include_map_) { - if (IsThirdPartyFile(item.first)) - third_party_headers_with_explicit_mappings.insert(item.first); - } - - set headers_included_from_non_third_party; - for (const auto& incmap : quoted_includes_to_quoted_includers_) { - for (const string& includer : incmap.second) { - if (!IsThirdPartyFile(includer)) { - headers_included_from_non_third_party.insert(incmap.first); - break; - } - } - } - - for (const auto& incmap : quoted_includes_to_quoted_includers_) { - const string& includee = incmap.first; - if (!IsThirdPartyFile(includee) || - ContainsKey(third_party_headers_with_explicit_mappings, includee) || - ContainsKey(headers_included_from_non_third_party, includee)) { - continue; - } - for (const string& includer : incmap.second) { - // From the 'if' statement above, we already know that includee - // is not included from non-third-party code. - CHECK_(IsThirdPartyFile(includer) && "Why not nixed!"); - CHECK_(IsThirdPartyFile(includee) && "Why not nixed!"); - AddMapping(includee, includer); - if (GetVisibility(includee) == kUnusedVisibility) { - MarkIncludeAsPrivate(includee); - } - } - } -} - // Handle work that's best done after we've seen all the mappings // (including dynamically-added ones) and all the include files. // For instance, we can now expand all the regexes we've seen in @@ -1307,10 +1250,6 @@ void IncludePicker::FinalizeAddedIncludes() { // Match those to seen #includes now. ExpandRegexes(); - // We treat third-party code specially, since it's difficult to add - // iwyu pragmas to code we don't own. - AddImplicitThirdPartyMappings(); - // If a.h maps to b.h maps to c.h, we'd like an entry from a.h to c.h too. MakeMapTransitive(&filepath_include_map_); // Now that filepath_include_map_ is transitively closed, it's an diff --git a/iwyu_include_picker.h b/iwyu_include_picker.h index cefe991..9e58c58 100644 --- a/iwyu_include_picker.h +++ b/iwyu_include_picker.h @@ -181,10 +181,6 @@ class IncludePicker { // seen by iwyu. void ExpandRegexes(); - // Figure out mappings to add between third-party files, that we - // guess based on the structure and use of third-party code. - void AddImplicitThirdPartyMappings(); - // Adds an entry to filepath_visibility_map_, with error checking. void MarkVisibility(const string& quoted_filepath_pattern, IncludeVisibility visibility); diff --git a/iwyu_path_util.cc b/iwyu_path_util.cc index cc8eed6..5d37367 100644 --- a/iwyu_path_util.cc +++ b/iwyu_path_util.cc @@ -225,20 +225,4 @@ bool IsSystemIncludeFile(const string& filepath) { return ConvertToQuotedInclude(filepath)[0] == '<'; } -// Returns true if the given file is third-party. Google-authored -// code living in third_party/ is not considered third-party. -bool IsThirdPartyFile(string quoted_path) { - if (!StripLeft("ed_path, "\"third_party/")) - return false; - - // These are Google-authored libraries living in third_party/ - // because of old licensing constraints. - if (StartsWith(quoted_path, "car/") || - StartsWith(quoted_path, "gtest/") || - StartsWith(quoted_path, "gmock/")) - return false; - - return true; -} - } // namespace include_what_you_use diff --git a/iwyu_path_util.h b/iwyu_path_util.h index c75667a..30cf2d4 100644 --- a/iwyu_path_util.h +++ b/iwyu_path_util.h @@ -88,10 +88,6 @@ bool IsQuotedInclude(const string& s); // file, based on where it lives. bool IsSystemIncludeFile(const string& filepath); -// Returns true if the given file is third-party. Google-authored -// code living in third_party/ is not considered third-party. -bool IsThirdPartyFile(string quoted_path); - } // namespace include_what_you_use #endif // INCLUDE_WHAT_YOU_USE_IWYU_PATH_UTIL_H_ diff --git a/iwyu_preprocessor.cc b/iwyu_preprocessor.cc index 71610c1..763192b 100644 --- a/iwyu_preprocessor.cc +++ b/iwyu_preprocessor.cc @@ -880,8 +880,7 @@ void IwyuPreprocessorInfo::PopulateIntendsToProvideMap() { } } // Ugh, we can have two files with the same name, using - // #include-next (e.g. /usr/include/c++/vector and - // third_party/gcc3/vector). Merge them. + // #include-next. Merge them. for (const auto& fileinfo : iwyu_file_info_map_) { const FileEntry* file = fileinfo.first; // See if a round-trip to string and back ends up at a different file. diff --git a/more_tests/iwyu_include_picker_test.cc b/more_tests/iwyu_include_picker_test.cc index 58a1c77..be55316 100644 --- a/more_tests/iwyu_include_picker_test.cc +++ b/more_tests/iwyu_include_picker_test.cc @@ -102,15 +102,12 @@ TEST(GetCanonicalName, MapsInternalToPublic) { TEST(IsSystemIncludeFile, Basic) { EXPECT_FALSE(IsSystemIncludeFile("foo.h")); - EXPECT_FALSE(IsSystemIncludeFile("third_party/ICU/icu.h")); EXPECT_TRUE(IsSystemIncludeFile("/usr/include/string.h")); EXPECT_TRUE(IsSystemIncludeFile("/usr/include/c++/4.3/bits/stl_vector.h")); } TEST(ConvertToQuotedInclude, Basic) { EXPECT_EQ("\"foo.h\"", ConvertToQuotedInclude("foo.h")); - EXPECT_EQ("\"third_party/ICU/icu.h\"", - ConvertToQuotedInclude("third_party/ICU/icu.h")); EXPECT_EQ("", ConvertToQuotedInclude("/usr/include/string.h")); EXPECT_EQ("", ConvertToQuotedInclude("/usr/include/c++/4.3/bits/stl_vector.h")); @@ -208,10 +205,6 @@ TEST(GetCandidateHeadersForFilepath, C) { EXPECT_VECTOR_STREQ( p.GetCandidateHeadersForFilepath("/usr/include/bits/dlfcn.h"), ""); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath("third_party/grte/v2/release/include/" - "bits/mathcalls.h"), - "", ""); EXPECT_VECTOR_STREQ( p.GetCandidateHeadersForFilepath("/usr/grte/v1/include/assert.h"), "", ""); @@ -226,95 +219,6 @@ TEST(GetCandidateHeadersForFilepath, CXX) { EXPECT_VECTOR_STREQ( p.GetCandidateHeadersForFilepath("/usr/include/c++/4.2/bits/allocator.h"), ""); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath( - "third_party/llvm/crosstool/gcc-4.4.0-glibc-2.3.6-grte/x86/" - "include/c++/4.4.0/backward/hash_fun.h"), - "", ""); -} - -TEST(IsThirdPartyFile, ReturnsFalseForGoogleFile) { - EXPECT_FALSE(IsThirdPartyFile("\"foo/bar.h\"")); - EXPECT_FALSE(IsThirdPartyFile("\"foo/third_party/bar.cc\"")); -} - -TEST(IsThirdPartyFile, ReturnsFalseForGoogleFileInThirdParty) { - EXPECT_FALSE(IsThirdPartyFile("\"third_party/car/car.h\"")); - EXPECT_FALSE(IsThirdPartyFile("\"third_party/gtest/a.h\"")); - EXPECT_FALSE(IsThirdPartyFile("\"third_party/gmock/b.h\"")); -} - -TEST(IsThirdPartyFile, ReturnsTrueForNonGoogleFileInThirdParty) { - EXPECT_TRUE(IsThirdPartyFile("\"third_party/tr1/tuple\"")); - EXPECT_TRUE(IsThirdPartyFile("\"third_party/foo/bar.h\"")); -} - -TEST(GetCandidateHeadersForFilepath, ThirdParty) { - IncludePicker p; - p.AddDirectInclude("a.h", "third_party/dynamic_annotations/d.h", ""); - p.AddDirectInclude("b.h", "third_party/dynamic_annotations/a/b/c.h", ""); - p.AddDirectInclude("c.h", "third_party/python_runtime/includes/py.h", ""); - p.AddDirectInclude("d.h", "third_party/isu/include/unicode/udraft.h", ""); - p.AddDirectInclude("e.h", "third_party/isu/include/unicode/ukeep.h", ""); - p.FinalizeAddedIncludes(); - - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath("third_party/dynamic_annotations/d.h"), - "\"base/dynamic_annotations.h\""); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath( - "third_party/dynamic_annotations/a/b/c.h"), - "\"base/dynamic_annotations.h\""); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath("third_party/python_runtime/includes/py.h"), - ""); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath( - "third_party/icu/include/unicode/udraft.h"), - "\"third_party/icu/include/unicode/utypes.h\""); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath( - "third_party/icu/include/unicode/ukeep.h"), - "\"third_party/icu/include/unicode/ukeep.h\""); -} - -TEST(GetCandidateHeadersForFilepath, ThirdPartyCycle) { - IncludePicker p; - // We should ignore the cycle here. - p.AddDirectInclude("myapp.h", "third_party/a.h", ""); - p.AddDirectInclude("third_party/a.h", "third_party/b.h", ""); - p.AddDirectInclude("third_party/b.h", "third_party/c.h", ""); - p.AddDirectInclude("third_party/c.h", "third_party/a.h", ""); // cycle! - p.FinalizeAddedIncludes(); - - // We ignore the cycle, so the includes look like a -> b -> c. In - // each case, a.h is the only public header file in the bunch; the - // rest are private because they're only #included from other - // third_party files. - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath("third_party/a.h"), - "\"third_party/a.h\""); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath("third_party/b.h"), - "\"third_party/a.h\""); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath("third_party/c.h"), - "\"third_party/a.h\""); -} - -TEST(GetCandidateHeadersForFilepath, RegexOverlap) { - IncludePicker p; - // It's ok if a header is specified in both a regex and non-regex rule. - // For regexes to work, we need to have actually seen the includes. - p.AddDirectInclude("a.h", "third_party/dynamic_annotations/d.h", ""); - p.AddMapping("\"third_party/dynamic_annotations/d.h\"", - "\"third_party/dynamic_annotations/public.h\""); - p.MarkIncludeAsPrivate("\"third_party/dynamic_annotations/d.h\""); - p.FinalizeAddedIncludes(); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath("third_party/dynamic_annotations/d.h"), - "\"third_party/dynamic_annotations/public.h\"", - "\"base/dynamic_annotations.h\""); } TEST(GetCandidateHeadersForFilepath, NoIdentityRegex) { @@ -336,105 +240,12 @@ TEST(GetCandidateHeadersForFilepath, NoIdentityRegex) { "\"mydir/include.h\""); } -TEST(GetCandidateHeadersForFilepath, ImplicitThirdPartyMapping) { - IncludePicker p; - p.AddDirectInclude("third_party/public.h", "third_party/private1.h", ""); - p.AddDirectInclude("third_party/public.h", "third_party/private2.h", ""); - p.AddDirectInclude("third_party/private1.h", "third_party/private11.h", ""); - p.AddDirectInclude("third_party/public.h", "third_party/other_public.h", ""); - p.AddDirectInclude("third_party/other_public.h", "third_party/oprivate.h", - ""); - p.AddDirectInclude("my_app.h", "third_party/public.h", ""); - p.AddDirectInclude("my_app.h", "third_party/other_public.h", ""); - p.FinalizeAddedIncludes(); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath("third_party/public.h"), - "\"third_party/public.h\""); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath("third_party/private1.h"), - "\"third_party/public.h\""); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath("third_party/private2.h"), - "\"third_party/public.h\""); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath("third_party/private11.h"), - "\"third_party/public.h\""); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath("third_party/other_public.h"), - "\"third_party/other_public.h\""); - // Shouldn't have third_party/public.h here, even though it indirectly - // includes oprivate.h, because other_public is in between and is itself - // considered a public include (since it is #included from my_app.h). - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath("third_party/oprivate.h"), - "\"third_party/other_public.h\""); -} - -TEST(GetCandidateHeadersForFilepath, TreatsGTestAsNonThirdParty) { - IncludePicker p; - p.AddDirectInclude("foo/foo.cc", "testing/base/public/gunit.h", ""); - p.AddDirectInclude("testing/base/public/gunit.h", - "third_party/gtest/include/gtest/gtest.h", ""); - p.FinalizeAddedIncludes(); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath("third_party/gtest/include/gtest/gtest.h"), - "\"third_party/gtest/include/gtest/gtest.h\""); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath("testing/base/public/gunit.h"), - "\"testing/base/public/gunit.h\""); -} - -TEST(GetCandidateHeadersForFilepath, ExplicitThirdPartyMapping) { - IncludePicker p; - // These are controlled by third_party_include_map, not by - // AddImplicitThirdPartyMappings(). - p.AddDirectInclude("my_app.h", "third_party/dynamic_annotations/public.h", - ""); - p.AddDirectInclude("third_party/dynamic_annotations/public.h", - "third_party/dynamic_annotations/private.h", ""); - p.AddDirectInclude("my_app.h", "third_party/icu/include/unicode/umachine.h", - ""); - p.FinalizeAddedIncludes(); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath( - "third_party/dynamic_annotations/public.h"), - "\"base/dynamic_annotations.h\""); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath( - "third_party/icu/include/unicode/umachine.h"), - "\"third_party/icu/include/unicode/utypes.h\""); -} - -TEST(GetCandidateHeadersForFilepath, ExplicitVsImplicitThirdPartyMapping) { - IncludePicker p; - // Here, the includees are all explicitly marked as public. - p.AddDirectInclude("my_app.h", "third_party/icu/include/unicode/foo.h", ""); - p.AddDirectInclude("third_party/icu/include/unicode/foo.h", - "third_party/icu/include/unicode/utypes.h", ""); - p.AddDirectInclude("third_party/icu/include/unicode/foo.h", - "/usr/include/stdio.h", ""); - p.FinalizeAddedIncludes(); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath("/usr/include/stdio.h"), - "", ""); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath( - "third_party/icu/include/unicode/utypes.h"), - "\"third_party/icu/include/unicode/utypes.h\"", - "\"third_party/icu/include/unicode/foo.h\""); -} - TEST(GetCandidateHeadersForFilepath, NotInAnyMap) { IncludePicker p; p.FinalizeAddedIncludes(); EXPECT_VECTOR_STREQ( p.GetCandidateHeadersForFilepath("/usr/grte/v1/include/poll.h"), ""); - EXPECT_VECTOR_STREQ( - p.GetCandidateHeadersForFilepath("third_party/llvm/crosstool/" - "gcc-4.4.0-glibc-2.3.6-grte/x86/" - "include/c++/4.4.0/vector"), - ""); EXPECT_VECTOR_STREQ( p.GetCandidateHeadersForFilepath("././././my/dot.h"), "\"my/dot.h\""); @@ -583,16 +394,6 @@ TEST(HasMapping, IncludeMatchDifferentMaps) { EXPECT_TRUE(p.HasMapping("/usr/include/c++/4.2/ios", "base/logging.h")); } -TEST(HasMapping, IncludeForThirdParty) { - IncludePicker p; - // For regexes to work, we need to have actually seen the includes. - p.AddDirectInclude("base/dynamic_annotations.h", - "third_party/dynamic_annotations/foo/bar.h", ""); - p.FinalizeAddedIncludes(); - EXPECT_TRUE(p.HasMapping("third_party/dynamic_annotations/foo/bar.h", - "base/dynamic_annotations.h")); -} - } // namespace } // namespace include_what_you_use diff --git a/third_party.imp b/third_party.imp deleted file mode 100644 index c5fc0bd..0000000 --- a/third_party.imp +++ /dev/null @@ -1,25 +0,0 @@ -# It's very common for third-party libraries to just expose one -# header file. So this map takes advantage of regex functionality. -# -# Please keep this in sync with _deprecated_headers in cpplint.py. -[ - { include: ["@\"third_party/dynamic_annotations/.*\"", private, "\"base/dynamic_annotations.h\"", public] }, - { include: ["@\"third_party/gmock/include/gmock/.*\"", private, "\"testing/base/public/gmock.h\"", public] }, - # Old location for Python. TODO(csilvers): remove when - # third_party/python2_4_3 is gone. - { include: ["@\"third_party/python2_4_3/.*\"", private, "", public] }, - # New location for Python. - { include: ["@\"third_party/python_runtime/.*\"", private, "", public] }, - { include: ["\"third_party/icu/include/unicode/umachine.h\"", private, "\"third_party/icu/include/unicode/utypes.h\"", public] }, - { include: ["\"third_party/icu/include/unicode/uversion.h\"", private, "\"third_party/icu/include/unicode/utypes.h\"", public] }, - { include: ["\"third_party/icu/include/unicode/uconfig.h\"", private, "\"third_party/icu/include/unicode/utypes.h\"", public] }, - { include: ["\"third_party/icu/include/unicode/udraft.h\"", private, "\"third_party/icu/include/unicode/utypes.h\"", public] }, - { include: ["\"third_party/icu/include/unicode/udeprctd.h\"", private, "\"third_party/icu/include/unicode/utypes.h\"", public] }, - { include: ["\"third_party/icu/include/unicode/uobslete.h\"", private, "\"third_party/icu/include/unicode/utypes.h\"", public] }, - { include: ["\"third_party/icu/include/unicode/uintrnal.h\"", private, "\"third_party/icu/include/unicode/utypes.h\"", public] }, - { include: ["\"third_party/icu/include/unicode/usystem.h\"", private, "\"third_party/icu/include/unicode/utypes.h\"", public] }, - { include: ["\"third_party/icu/include/unicode/urename.h\"", private, "\"third_party/icu/include/unicode/utypes.h\"", public] }, - { include: ["\"third_party/icu/include/unicode/platform.h\"", private, "\"third_party/icu/include/unicode/utypes.h\"", public] }, - { include: ["\"third_party/icu/include/unicode/ptypes.h\"", private, "\"third_party/icu/include/unicode/utypes.h\"", public] }, - { include: ["\"third_party/icu/include/unicode/uvernum.h\"", private, "\"third_party/icu/include/unicode/utypes.h\"", public] }, -]