From 1604716dfc493bf55bc1d87b5083bbe2ee709e60 Mon Sep 17 00:00:00 2001 From: Brad King Date: Mon, 25 Sep 2017 08:33:29 -0400 Subject: Revert "Performance: Improve efficiency of source file lookup in cmMakefile" This reverts commit 3b95ab569345028a1a8fe521d5ecd81fa97f2653. It regressed some legacy source file property behavior. Revert pending further investigation. --- Source/cmMakefile.cxx | 50 +++----------------------------------------------- Source/cmMakefile.h | 11 ----------- 2 files changed, 3 insertions(+), 58 deletions(-) diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index c96b892..e51cfcc 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -3082,18 +3082,9 @@ void cmMakefile::SetArgcArgv(const std::vector& args) cmSourceFile* cmMakefile::GetSource(const std::string& sourceName) const { cmSourceFileLocation sfl(this, sourceName); - -#if defined(_WIN32) || defined(__APPLE__) - const auto& name = cmSystemTools::LowerCase(sfl.GetName()); -#else - const auto& name = sfl.GetName(); -#endif - auto sfsi = this->SourceFileSearchIndex.find(name); - if (sfsi != this->SourceFileSearchIndex.end()) { - for (auto sf : sfsi->second) { - if (sf->Matches(sfl)) { - return sf; - } + for (cmSourceFile* sf : this->SourceFiles) { + if (sf->Matches(sfl)) { + return sf; } } return nullptr; @@ -3107,41 +3098,6 @@ cmSourceFile* cmMakefile::CreateSource(const std::string& sourceName, sf->SetProperty("GENERATED", "1"); } this->SourceFiles.push_back(sf); - - auto name = sf->GetLocation().GetName(); -#if defined(_WIN32) || defined(__APPLE__) - name = cmSystemTools::LowerCase(name); -#endif - - // For a file in the form "a.b.c" add the cmSourceFile to the index - // at "a.b.c", "a.b" and "a". - auto partial = name; - while (true) { - this->SourceFileSearchIndex[partial].insert(sf); - auto i = partial.rfind('.'); - if (i == std::string::npos) { - break; - } - partial = partial.substr(0, i); - } - - if (sf->GetLocation().ExtensionIsAmbiguous()) { - // For an ambiguous extension also add the various "known" - // extensions to the original filename. - - const auto& srcExts = this->GetCMakeInstance()->GetSourceExtensions(); - for (const auto& ext : srcExts) { - auto name_ext = name + "." + cmSystemTools::LowerCase(ext); - this->SourceFileSearchIndex[name_ext].insert(sf); - } - - const auto& hdrExts = this->GetCMakeInstance()->GetHeaderExtensions(); - for (const auto& ext : hdrExts) { - auto name_ext = name + "." + cmSystemTools::LowerCase(ext); - this->SourceFileSearchIndex[name_ext].insert(sf); - } - } - return sf; } diff --git a/Source/cmMakefile.h b/Source/cmMakefile.h index 2cae659..272522c 100644 --- a/Source/cmMakefile.h +++ b/Source/cmMakefile.h @@ -13,7 +13,6 @@ #include #include #include -#include #include #include "cmAlgorithms.h" @@ -809,17 +808,7 @@ protected: // libraries, classes, and executables mutable cmTargets Targets; std::map AliasTargets; - std::vector SourceFiles; - // Because cmSourceFile names are compared in a fuzzy way (see - // cmSourceFileLocation::Match()) we can't have a straight mapping from - // filename to cmSourceFile. To make lookups more efficient we store the - // Name portion of the cmSourceFileLocation and then compare on the list of - // cmSourceFiles that might match that name. Note that on platforms which - // have a case-insensitive filesystem we store the key in all lowercase. - typedef std::unordered_set SourceFileSet; - typedef std::unordered_map SourceFileMap; - SourceFileMap SourceFileSearchIndex; // Tests std::map Tests; -- cgit v0.12 From a7005c985d2ed22f1192864a358eb89e2c1b63f7 Mon Sep 17 00:00:00 2001 From: Brad King Date: Mon, 25 Sep 2017 08:32:29 -0400 Subject: Tests: Add case for legacy source file property behavior The change in commit 3b95ab5693 (Performance: Improve efficiency of source file lookup in cmMakefile, 2017-08-17) broke some legacy behavior of source file properties in which the order sources are first resolved with extensions affects how setting properties without extensions works. It has been reverted for now, but the discovery was made after merging because the broken case was not covered by our test suite. Add a test case representing the legacy behavior. Issue: #15208 --- Tests/RunCMake/get_property/source_properties-stderr.txt | 8 +++++++- Tests/RunCMake/get_property/source_properties.cmake | 10 ++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/Tests/RunCMake/get_property/source_properties-stderr.txt b/Tests/RunCMake/get_property/source_properties-stderr.txt index 0a46f96..00a9b82 100644 --- a/Tests/RunCMake/get_property/source_properties-stderr.txt +++ b/Tests/RunCMake/get_property/source_properties-stderr.txt @@ -3,4 +3,10 @@ get_property: --><-- get_source_file_property: -->value<-- get_property: -->value<-- get_source_file_property: -->NOTFOUND<-- -get_property: --><--$ +get_property: --><-- +get_source_file_property: -->value<-- +get_property: -->value<-- +get_source_file_property: -->NOTFOUND<-- +get_property: --><-- +get_source_file_property: -->value<-- +get_property: -->value<--$ diff --git a/Tests/RunCMake/get_property/source_properties.cmake b/Tests/RunCMake/get_property/source_properties.cmake index 263ffe1..12d2d07 100644 --- a/Tests/RunCMake/get_property/source_properties.cmake +++ b/Tests/RunCMake/get_property/source_properties.cmake @@ -13,3 +13,13 @@ set_source_files_properties(file.c PROPERTIES empty "" custom value) check_source_file_property(file.c empty) check_source_file_property(file.c custom) check_source_file_property(file.c noexist) + +# Test strange legacy behavior in which the order in which source files are +# first accessed affects how properties are applied without an extension. +# See also issue #15208. +get_property(lang SOURCE ${CMAKE_CURRENT_BINARY_DIR}/file2.c PROPERTY LANGUAGE) +get_property(lang SOURCE ${CMAKE_CURRENT_BINARY_DIR}/file2.h PROPERTY LANGUAGE) +set_property(SOURCE file2 PROPERTY custom value) # set property without extension +check_source_file_property(file2 custom) # should have property +check_source_file_property(file2.h custom) # should not have property +check_source_file_property(file2.c custom) # should have property -- cgit v0.12