From f16dfdf71f8f81e1be9c3ea18652d8d87c1cb265 Mon Sep 17 00:00:00 2001 From: Sebastian Holtermann Date: Fri, 25 Jan 2019 15:03:31 +0100 Subject: cmLocalGenerator: Simplify `GetIncludeDirectories` This patch strips the `stripImplicitDirs` and `appendAllImplicitDirs` parameters from the `cmLocalGenerator::GetIncludeDirectories` method and makes it a wrapper into the new `cmLocalGenerator::GetIncludeDirectoriesImplicit` method. `cmLocalGenerator::GetIncludeDirectoriesImplicit` is the renamed old implementation of `cmLocalGenerator::GetIncludeDirectories` and still accepts `stripImplicitDirs` and `appendAllImplicitDirs`. The motivation is that there's only *one* case where `cmLocalGenerator::GetIncludeDirectories` is called with the `stripImplicitDirs` parameter being `false` (QtAutoGen), but many other places where it is called using the `true` default value. QtAutoGen is modified to use `cmLocalGenerator::GetIncludeDirectoriesImplicit` directly. In two use cases of `cmLocalGenerator::GetIncludeDirectories` the manually set `true` value for `stripImplicitDirs` is removed. --- Source/cmFileAPICodemodel.cxx | 2 +- Source/cmJsonObjects.cxx | 2 +- Source/cmLocalGenerator.cxx | 45 +++++++++++++++++++++++++-------------- Source/cmLocalGenerator.h | 36 +++++++++++++++++++++++++------ Source/cmQtAutoGenInitializer.cxx | 4 ++-- 5 files changed, 63 insertions(+), 26 deletions(-) diff --git a/Source/cmFileAPICodemodel.cxx b/Source/cmFileAPICodemodel.cxx index 3363c9b..4597d4f 100644 --- a/Source/cmFileAPICodemodel.cxx +++ b/Source/cmFileAPICodemodel.cxx @@ -727,7 +727,7 @@ void Target::ProcessLanguage(std::string const& lang) lg->GetTargetDefines(this->GT, this->Config, lang); cd.SetDefines(defines); std::vector> includePathList = - lg->GetIncludeDirectories(this->GT, lang, this->Config, true); + lg->GetIncludeDirectories(this->GT, lang, this->Config); for (BT const& i : includePathList) { cd.Includes.emplace_back( i, this->GT->IsSystemIncludeDirectory(i.Value, this->Config, lang)); diff --git a/Source/cmJsonObjects.cxx b/Source/cmJsonObjects.cxx index 135fd25..4b6b4d5 100644 --- a/Source/cmJsonObjects.cxx +++ b/Source/cmJsonObjects.cxx @@ -578,7 +578,7 @@ static Json::Value DumpTarget(cmGeneratorTarget* target, lg->GetTargetDefines(target, config, lang, defines); ld.SetDefines(defines); std::vector includePathList; - lg->GetIncludeDirectories(includePathList, target, lang, config, true); + lg->GetIncludeDirectories(includePathList, target, lang, config); for (std::string const& i : includePathList) { ld.IncludePathList.emplace_back( i, target->IsSystemIncludeDirectory(i, config, lang)); diff --git a/Source/cmLocalGenerator.cxx b/Source/cmLocalGenerator.cxx index 62aff99..da72d9d 100644 --- a/Source/cmLocalGenerator.cxx +++ b/Source/cmLocalGenerator.cxx @@ -881,22 +881,7 @@ void cmLocalGenerator::AddCompileOptions(std::string& flags, this->AddCompilerRequirementFlag(flags, target, lang); } -void cmLocalGenerator::GetIncludeDirectories(std::vector& dirs, - cmGeneratorTarget const* target, - const std::string& lang, - const std::string& config, - bool stripImplicitDirs, - bool appendAllImplicitDirs) const -{ - std::vector> tmp = this->GetIncludeDirectories( - target, lang, config, stripImplicitDirs, appendAllImplicitDirs); - dirs.reserve(tmp.size()); - for (BT& v : tmp) { - dirs.emplace_back(std::move(v.Value)); - } -} - -std::vector> cmLocalGenerator::GetIncludeDirectories( +std::vector> cmLocalGenerator::GetIncludeDirectoriesImplicit( cmGeneratorTarget const* target, std::string const& lang, std::string const& config, bool stripImplicitDirs, bool appendAllImplicitDirs) const @@ -1043,6 +1028,34 @@ std::vector> cmLocalGenerator::GetIncludeDirectories( return result; } +void cmLocalGenerator::GetIncludeDirectoriesImplicit( + std::vector& dirs, cmGeneratorTarget const* target, + const std::string& lang, const std::string& config, bool stripImplicitDirs, + bool appendAllImplicitDirs) const +{ + std::vector> tmp = this->GetIncludeDirectoriesImplicit( + target, lang, config, stripImplicitDirs, appendAllImplicitDirs); + dirs.reserve(dirs.size() + tmp.size()); + for (BT& v : tmp) { + dirs.emplace_back(std::move(v.Value)); + } +} + +std::vector> cmLocalGenerator::GetIncludeDirectories( + cmGeneratorTarget const* target, std::string const& lang, + std::string const& config) const +{ + return this->GetIncludeDirectoriesImplicit(target, lang, config); +} + +void cmLocalGenerator::GetIncludeDirectories(std::vector& dirs, + cmGeneratorTarget const* target, + const std::string& lang, + const std::string& config) const +{ + this->GetIncludeDirectoriesImplicit(dirs, target, lang, config); +} + void cmLocalGenerator::GetStaticLibraryFlags(std::string& flags, std::string const& config, std::string const& linkLanguage, diff --git a/Source/cmLocalGenerator.h b/Source/cmLocalGenerator.h index ebc613b..7cc7e37 100644 --- a/Source/cmLocalGenerator.h +++ b/Source/cmLocalGenerator.h @@ -239,22 +239,46 @@ public: return true; } - /** @brief Get the include directories for the current makefile and language. + /** @brief Get the include directories for the current makefile and language + * and optional the compiler implicit include directories. + * * @arg stripImplicitDirs Strip all directories found in * CMAKE__IMPLICIT_INCLUDE_DIRECTORIES from the result. * @arg appendAllImplicitDirs Append all directories found in * CMAKE__IMPLICIT_INCLUDE_DIRECTORIES to the result. */ + std::vector> GetIncludeDirectoriesImplicit( + cmGeneratorTarget const* target, std::string const& lang = "C", + std::string const& config = "", bool stripImplicitDirs = true, + bool appendAllImplicitDirs = false) const; + + /** @brief Get the include directories for the current makefile and language + * and optional the compiler implicit include directories. + * + * @arg dirs Directories are appended to this list + */ + void GetIncludeDirectoriesImplicit(std::vector& dirs, + cmGeneratorTarget const* target, + const std::string& lang = "C", + const std::string& config = "", + bool stripImplicitDirs = true, + bool appendAllImplicitDirs = false) const; + + /** @brief Get the include directories for the current makefile and language. + * @arg dirs Include directories are appended to this list + */ void GetIncludeDirectories(std::vector& dirs, cmGeneratorTarget const* target, const std::string& lang = "C", - const std::string& config = "", - bool stripImplicitDirs = true, - bool appendAllImplicitDirs = false) const; + const std::string& config = "") const; + + /** @brief Get the include directories for the current makefile and language. + * @return The include directory list + */ std::vector> GetIncludeDirectories( cmGeneratorTarget const* target, std::string const& lang = "C", - std::string const& config = "", bool stripImplicitDirs = true, - bool appendAllImplicitDirs = false) const; + std::string const& config = "") const; + void AddCompileOptions(std::string& flags, cmGeneratorTarget* target, const std::string& lang, const std::string& config); diff --git a/Source/cmQtAutoGenInitializer.cxx b/Source/cmQtAutoGenInitializer.cxx index e0795d2..817f0b7 100644 --- a/Source/cmQtAutoGenInitializer.cxx +++ b/Source/cmQtAutoGenInitializer.cxx @@ -531,8 +531,8 @@ bool cmQtAutoGenInitializer::InitMoc() // include dirs off, see // https://gitlab.kitware.com/cmake/cmake/issues/13667 std::vector dirs; - localGen->GetIncludeDirectories(dirs, this->Target, "CXX", cfg, false, - appendImplicit); + localGen->GetIncludeDirectoriesImplicit(dirs, this->Target, "CXX", cfg, + false, appendImplicit); return dirs; }; -- cgit v0.12 From 5f34bdc7f9f1fd8cca69757b06b3de8fdb4930b7 Mon Sep 17 00:00:00 2001 From: Sebastian Holtermann Date: Fri, 25 Jan 2019 18:21:35 +0100 Subject: cmLocalGenerator: Refactor `GetIncludeDirectoriesImplicit` method Use a dedicated `std::set` for implicit include directories instead of (ab)using the emitted variable. This in combination with lambdas makes it easier to comprehend which paths are emitted. For the implicit include directories we used to omit the `CMAKE_SYSROOT_COMPILE`/`CMAKE_SYSROOT` prefix. This has been changed and the implicit paths are returned prefixed. Implicit include directory returning is only ever used by QtAutoGen which requires prefixed paths. QtAutoGen passes the (implicit) include paths to the `moc` which isn't aware of `CMAKE_SYSROOT_COMPILE`/`CMAKE_SYSROOT`. --- Source/cmLocalGenerator.cxx | 96 ++++++++++++++++++++++++--------------------- 1 file changed, 51 insertions(+), 45 deletions(-) diff --git a/Source/cmLocalGenerator.cxx b/Source/cmLocalGenerator.cxx index da72d9d..1122924 100644 --- a/Source/cmLocalGenerator.cxx +++ b/Source/cmLocalGenerator.cxx @@ -887,10 +887,21 @@ std::vector> cmLocalGenerator::GetIncludeDirectoriesImplicit( bool appendAllImplicitDirs) const { std::vector> result; - // Do not repeat an include path. std::set emitted; + auto emitDir = [&result, &emitted](std::string const& dir) { + if (emitted.insert(dir).second) { + result.emplace_back(dir); + } + }; + + auto emitBT = [&result, &emitted](BT const& dir) { + if (emitted.insert(dir.Value).second) { + result.emplace_back(dir); + } + }; + // When automatic include directories are requested for a build then // include the source and binary directories at the beginning of the // include path to approximate include file behavior for an @@ -900,21 +911,9 @@ std::vector> cmLocalGenerator::GetIncludeDirectoriesImplicit( // per-source-file include paths. if (this->Makefile->IsOn("CMAKE_INCLUDE_CURRENT_DIR")) { // Current binary directory - { - std::string binDir = - this->StateSnapshot.GetDirectory().GetCurrentBinary(); - if (emitted.insert(binDir).second) { - result.emplace_back(std::move(binDir)); - } - } + emitDir(this->StateSnapshot.GetDirectory().GetCurrentBinary()); // Current source directory - { - std::string srcDir = - this->StateSnapshot.GetDirectory().GetCurrentSource(); - if (emitted.insert(srcDir).second) { - result.emplace_back(std::move(srcDir)); - } - } + emitDir(this->StateSnapshot.GetDirectory().GetCurrentSource()); } if (!target) { @@ -923,6 +922,11 @@ std::vector> cmLocalGenerator::GetIncludeDirectoriesImplicit( // Implicit include directories std::vector implicitDirs; + std::set implicitSet; + // Checks if this is not an implicit include directory + auto notImplicit = [&implicitSet](std::string const& dir) { + return (implicitSet.find(dir) == implicitSet.end()); + }; { std::string rootPath; if (const char* sysrootCompile = @@ -932,6 +936,7 @@ std::vector> cmLocalGenerator::GetIncludeDirectoriesImplicit( rootPath = this->Makefile->GetSafeDefinition("CMAKE_SYSROOT"); } + // Raw list of implicit include directories std::vector impDirVec; // Get platform-wide implicit directories. @@ -949,12 +954,11 @@ std::vector> cmLocalGenerator::GetIncludeDirectoriesImplicit( } for (std::string const& i : impDirVec) { - { - std::string d = rootPath + i; - cmSystemTools::ConvertToUnixSlashes(d); - emitted.insert(std::move(d)); + std::string imd = rootPath + i; + cmSystemTools::ConvertToUnixSlashes(imd); + if (implicitSet.insert(imd).second) { + implicitDirs.emplace_back(std::move(imd)); } - implicitDirs.push_back(i); } } @@ -967,30 +971,31 @@ std::vector> cmLocalGenerator::GetIncludeDirectoriesImplicit( if (this->Makefile->IsOn("CMAKE_INCLUDE_DIRECTORIES_PROJECT_BEFORE")) { std::string const &topSourceDir = this->GetState()->GetSourceDirectory(), &topBinaryDir = this->GetState()->GetBinaryDirectory(); - for (BT const& i : userDirs) { + for (BT const& udr : userDirs) { // Emit this directory only if it is a subdirectory of the // top-level source or binary tree. - if (cmSystemTools::ComparePath(i.Value, topSourceDir) || - cmSystemTools::ComparePath(i.Value, topBinaryDir) || - cmSystemTools::IsSubDirectory(i.Value, topSourceDir) || - cmSystemTools::IsSubDirectory(i.Value, topBinaryDir)) { - if (emitted.insert(i.Value).second) { - result.push_back(i); + if (cmSystemTools::ComparePath(udr.Value, topSourceDir) || + cmSystemTools::ComparePath(udr.Value, topBinaryDir) || + cmSystemTools::IsSubDirectory(udr.Value, topSourceDir) || + cmSystemTools::IsSubDirectory(udr.Value, topBinaryDir)) { + if (notImplicit(udr.Value)) { + emitBT(udr); } } } } - // Construct the final ordered include directory list. - for (BT const& i : userDirs) { - if (emitted.insert(i.Value).second) { - result.push_back(i); + // Emit remaining non implicit user direcories. + for (BT const& udr : userDirs) { + if (notImplicit(udr.Value)) { + emitBT(udr); } } + // Sort result MoveSystemIncludesToEnd(result, config, lang, target); - // Add standard include directories for this language. + // Append standard include directories for this language. { std::vector userStandardDirs; { @@ -1001,26 +1006,27 @@ std::vector> cmLocalGenerator::GetIncludeDirectoriesImplicit( cmSystemTools::ExpandListArgument(value, userStandardDirs); } userDirs.reserve(userDirs.size() + userStandardDirs.size()); - for (std::string& d : userStandardDirs) { - cmSystemTools::ConvertToUnixSlashes(d); - result.emplace_back(d); - userDirs.emplace_back(std::move(d)); + for (std::string& usd : userStandardDirs) { + cmSystemTools::ConvertToUnixSlashes(usd); + if (notImplicit(usd)) { + emitDir(usd); + } + userDirs.emplace_back(std::move(usd)); } } + // Append compiler implicit include directories if (!stripImplicitDirs) { - // Append only implicit directories that were requested by the user - for (std::string const& i : implicitDirs) { - if (std::find(userDirs.begin(), userDirs.end(), i) != userDirs.end()) { - result.emplace_back(i); + // Append implicit directories that were requested by the user only + for (BT const& udr : userDirs) { + if (!notImplicit(udr.Value)) { + emitBT(udr); } } - // Append remaining implicit directories on demand + // Append remaining implicit directories (on demand) if (appendAllImplicitDirs) { - for (std::string const& i : implicitDirs) { - if (std::find(result.begin(), result.end(), i) == result.end()) { - result.emplace_back(i); - } + for (std::string& imd : implicitDirs) { + emitDir(imd); } } } -- cgit v0.12 From 03dbb62d31d4a115fed09049cf6995ce89d2fabe Mon Sep 17 00:00:00 2001 From: Sebastian Holtermann Date: Fri, 25 Jan 2019 19:09:45 +0100 Subject: Autogen: Reenable passing compiler implicit include directories to moc Since commit 5990ecb741 (Compute implicit include directories from compiler output, 2018-12-07) we now have compiler implicit include directory computation for gcc and clang. It should be safe now to pass these to `moc`. This patch re-enables passing the compiler implicit include directories to `moc`, which was disabled due to issue #18669. Fixes: #18041 Issue: #18669 --- Source/cmQtAutoGenInitializer.cxx | 9 +++------ Tests/QtAutogen/MocOsMacros/TestClass.cpp | 5 +++++ Tests/QtAutogen/MocOsMacros/TestClass.hpp | 5 +++++ Tests/QtAutogen/Tests.cmake | 3 +-- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/Source/cmQtAutoGenInitializer.cxx b/Source/cmQtAutoGenInitializer.cxx index 817f0b7..1d7f6d1 100644 --- a/Source/cmQtAutoGenInitializer.cxx +++ b/Source/cmQtAutoGenInitializer.cxx @@ -520,13 +520,10 @@ bool cmQtAutoGenInitializer::InitMoc() // Moc includes { - // We need to disable this until we have all implicit includes available. - // See issue #18669. - // bool const appendImplicit = (this->QtVersion.Major >= 5); - + bool const appendImplicit = (this->QtVersion.Major >= 5); auto GetIncludeDirs = - [this, localGen](std::string const& cfg) -> std::vector { - bool const appendImplicit = false; + [this, localGen, + appendImplicit](std::string const& cfg) -> std::vector { // Get the include dirs for this target, without stripping the implicit // include dirs off, see // https://gitlab.kitware.com/cmake/cmake/issues/13667 diff --git a/Tests/QtAutogen/MocOsMacros/TestClass.cpp b/Tests/QtAutogen/MocOsMacros/TestClass.cpp index 340d130..13f8fd9 100644 --- a/Tests/QtAutogen/MocOsMacros/TestClass.cpp +++ b/Tests/QtAutogen/MocOsMacros/TestClass.cpp @@ -1,6 +1,11 @@ #include "TestClass.hpp" #include +void TestClass::open() +{ + std::cout << "open\n"; +} + // -- Mac #ifndef Q_OS_MAC void TestClass::MacNotDef() diff --git a/Tests/QtAutogen/MocOsMacros/TestClass.hpp b/Tests/QtAutogen/MocOsMacros/TestClass.hpp index 53000aa..87fd494 100644 --- a/Tests/QtAutogen/MocOsMacros/TestClass.hpp +++ b/Tests/QtAutogen/MocOsMacros/TestClass.hpp @@ -3,12 +3,17 @@ #include #include +// include qplatformdefs.h for #18669 +#include class TestClass : public QObject { Q_OBJECT public Q_SLOTS: + // Method named "open" to test if #18669 is fixed + void open(); + // -- Mac #ifndef Q_OS_MAC void MacNotDef(); diff --git a/Tests/QtAutogen/Tests.cmake b/Tests/QtAutogen/Tests.cmake index c53fb4f..096d5e3 100644 --- a/Tests/QtAutogen/Tests.cmake +++ b/Tests/QtAutogen/Tests.cmake @@ -39,8 +39,7 @@ endif() # Qt5 only tests if(QT_TEST_VERSION GREATER 4) ADD_AUTOGEN_TEST(MocMacroName mocMacroName) - # Disabled for issue #18669 - #ADD_AUTOGEN_TEST(MocOsMacros) + ADD_AUTOGEN_TEST(MocOsMacros) ADD_AUTOGEN_TEST(RerunMocPlugin) if(APPLE) ADD_AUTOGEN_TEST(MacOsFW) -- cgit v0.12