ClangPchManager: Don't update system PCH if no system include file changed

If a project or user file is touched the system PCH should not be
regenerated.

Change-Id: Id5989735bab4441fc1eebfb5d31e01ba9714428b
Reviewed-by: Tim Jenssen <tim.jenssen@qt.io>

src/tools/clangpchmanagerbackend/clangpchmanagerbackendmain.cpp

@@ -221,7 +221,8 @@ struct Data // because we have a cycle dependency
                                                                     database};
     ClangBackEnd::PchTaskGenerator pchTaskGenerator{buildDependencyProvider,
                                                     pchTaskMerger,
-                                                    dependencyCreationProgressCounter};
+                                                    dependencyCreationProgressCounter,
+                                                    pchTaskQueue};
     PchManagerServer clangPchManagerServer{includeWatcher,
                                            pchTaskGenerator,
                                            projectParts,

src/tools/clangpchmanagerbackend/source/pchmanagerserver.cpp

@@ -143,46 +143,67 @@ struct FilterResults
     ProjectPartIds userIds;
 };
 
-std::pair<ProjectPartIds, ProjectPartIds> pchProjectPartIds(const std::vector<IdPaths> &idPaths)
+ProjectPartIds removeIds(const ProjectPartIds &subtrahend, const ProjectPartIds &minuend)
 {
-    ProjectPartIds changedProjectPartIds;
+    ProjectPartIds difference;
-    changedProjectPartIds.reserve(idPaths.size());
+    difference.reserve(subtrahend.size());
 
-    ProjectPartIds changedPchProjectPartIds;
+    std::set_difference(subtrahend.begin(),
-    changedPchProjectPartIds.reserve(idPaths.size());
+                        subtrahend.end(),
+                        minuend.begin(),
+                        minuend.end(),
+                        std::back_inserter(difference));
+
+    return difference;
+}
+
+FilterResults pchProjectPartIds(const std::vector<IdPaths> &idPaths)
+{
+    ProjectPartIds changedUserProjectPartIds;
+    changedUserProjectPartIds.reserve(idPaths.size());
+
+    ProjectPartIds changedSystemPchProjectPartIds;
+    changedSystemPchProjectPartIds.reserve(idPaths.size());
+
+    ProjectPartIds changedProjectPchProjectPartIds;
+    changedProjectPchProjectPartIds.reserve(idPaths.size());
 
     for (const IdPaths &idPath : idPaths) {
         switch (idPath.id.sourceType) {
         case SourceType::TopSystemInclude:
         case SourceType::SystemInclude:
+            changedSystemPchProjectPartIds.push_back(idPath.id.id);
+            break;
         case SourceType::TopProjectInclude:
         case SourceType::ProjectInclude:
-            changedPchProjectPartIds.push_back(idPath.id.id);
+            changedProjectPchProjectPartIds.push_back(idPath.id.id);
             break;
         case SourceType::UserInclude:
         case SourceType::Source:
-            changedProjectPartIds.push_back(idPath.id.id);
+            changedUserProjectPartIds.push_back(idPath.id.id);
             break;
         }
     }
 
-    changedPchProjectPartIds.erase(std::unique(changedPchProjectPartIds.begin(),
+    changedSystemPchProjectPartIds.erase(std::unique(changedSystemPchProjectPartIds.begin(),
-                                               changedPchProjectPartIds.end()),
+                                                     changedSystemPchProjectPartIds.end()),
-                                   changedPchProjectPartIds.end());
+                                         changedSystemPchProjectPartIds.end());
-    changedPchProjectPartIds.erase(std::unique(changedPchProjectPartIds.begin(),
+    changedProjectPchProjectPartIds.erase(std::unique(changedProjectPchProjectPartIds.begin(),
-                                               changedPchProjectPartIds.end()),
+                                                      changedProjectPchProjectPartIds.end()),
-                                   changedPchProjectPartIds.end());
+                                          changedProjectPchProjectPartIds.end());
+    changedUserProjectPartIds.erase(std::unique(changedUserProjectPartIds.begin(),
+                                                changedUserProjectPartIds.end()),
+                                    changedUserProjectPartIds.end());
 
-    ProjectPartIds changedUserProjectPartIds;
+    changedProjectPchProjectPartIds = removeIds(changedProjectPchProjectPartIds,
-    changedProjectPartIds.reserve(changedProjectPartIds.size());
+                                                changedSystemPchProjectPartIds);
 
-    std::set_difference(changedProjectPartIds.begin(),
+    changedUserProjectPartIds = removeIds(changedUserProjectPartIds, changedSystemPchProjectPartIds);
-                        changedProjectPartIds.end(),
+    changedUserProjectPartIds = removeIds(changedUserProjectPartIds, changedProjectPchProjectPartIds);
-                        changedPchProjectPartIds.begin(),
-                        changedPchProjectPartIds.end(),
-                        std::back_inserter(changedUserProjectPartIds));
 
-    return {changedPchProjectPartIds, changedUserProjectPartIds};
+    return {std::move(changedSystemPchProjectPartIds),
+            std::move(changedProjectPchProjectPartIds),
+            std::move(changedUserProjectPartIds)};
 }
 } // namespace
 
@@ -190,14 +211,11 @@ void PchManagerServer::pathsWithIdsChanged(const std::vector<IdPaths> &idPaths)
 {
     auto changedProjectPartIds = pchProjectPartIds(idPaths);
 
-    ArgumentsEntries entries = m_toolChainsArgumentsCache.arguments(changedProjectPartIds.first);
+    addCompleteProjectParts(changedProjectPartIds.systemIds);
 
-    for (ArgumentsEntry &entry : entries) {
+    addNonSystemProjectParts(changedProjectPartIds.projectIds);
-        m_pchTaskGenerator.addProjectParts(m_projectPartsManager.projects(entry.ids),
-                                           std::move(entry.arguments));
-    }
 
-    client()->precompiledHeadersUpdated(std::move(changedProjectPartIds.second));
+    client()->precompiledHeadersUpdated(std::move(changedProjectPartIds.userIds));
 }
 
 void PchManagerServer::pathsChanged(const FilePathIds &filePathIds)
@@ -215,4 +233,24 @@ void PchManagerServer::setDependencyCreationProgress(int progress, int total)
     client()->progress({ProgressType::DependencyCreation, progress, total});
 }
 
+void PchManagerServer::addCompleteProjectParts(const ProjectPartIds &projectPartIds)
+{
+    ArgumentsEntries entries = m_toolChainsArgumentsCache.arguments(projectPartIds);
+
+    for (ArgumentsEntry &entry : entries) {
+        m_pchTaskGenerator.addProjectParts(m_projectPartsManager.projects(entry.ids),
+                                           std::move(entry.arguments));
+    }
+}
+
+void PchManagerServer::addNonSystemProjectParts(const ProjectPartIds &projectPartIds)
+{
+    ArgumentsEntries entries = m_toolChainsArgumentsCache.arguments(projectPartIds);
+
+    for (ArgumentsEntry &entry : entries) {
+        m_pchTaskGenerator.addNonSystemProjectParts(m_projectPartsManager.projects(entry.ids),
+                                                    std::move(entry.arguments));
+    }
+}
+
 } // namespace ClangBackEnd

src/tools/clangpchmanagerbackend/source/pchmanagerserver.h

@@ -65,6 +65,10 @@ public:
     void setPchCreationProgress(int progress, int total);
     void setDependencyCreationProgress(int progress, int total);
 
+private:
+    void addCompleteProjectParts(const ProjectPartIds &projectPartIds);
+    void addNonSystemProjectParts(const ProjectPartIds &projectPartIds);
+
 private:
     ClangPathWatcherInterface &m_fileSystemWatcher;
     PchTaskGeneratorInterface &m_pchTaskGenerator;

src/tools/clangpchmanagerbackend/source/pchtaskgenerator.cpp

@@ -26,6 +26,7 @@
 #include "pchtaskgenerator.h"
 
 #include "builddependenciesproviderinterface.h"
+#include "pchtaskqueueinterface.h"
 #include "pchtasksmergerinterface.h"
 #include "usedmacrofilter.h"
 
@@ -88,4 +89,38 @@ void PchTaskGenerator::removeProjectParts(const ProjectPartIds &projectsPartIds)
     m_pchTasksMergerInterface.removePchTasks(projectsPartIds);
 }
 
+void PchTaskGenerator::addNonSystemProjectParts(ProjectPartContainers &&projectParts,
+                                                Utils::SmallStringVector &&)
+{
+    PchTasks pchTasks;
+    pchTasks.reserve(projectParts.size());
+
+    m_progressCounter.addTotal(static_cast<int>(projectParts.size()));
+
+    for (auto &projectPart : projectParts) {
+        BuildDependency buildDependency = m_buildDependenciesProvider.create(projectPart);
+        UsedMacroFilter filter{buildDependency.sources,
+                               buildDependency.usedMacros,
+                               projectPart.compilerMacros};
+
+        pchTasks.emplace_back(projectPart.projectPartId,
+                              std::move(filter.topProjectIncludes),
+                              std::move(filter.systemIncludes),
+                              std::move(filter.projectIncludes),
+                              std::move(filter.userIncludes),
+                              std::move(filter.sources),
+                              std::move(filter.projectCompilerMacros),
+                              std::move(filter.projectUsedMacros),
+                              projectPart.toolChainArguments,
+                              projectPart.systemIncludeSearchPaths,
+                              projectPart.projectIncludeSearchPaths,
+                              projectPart.language,
+                              projectPart.languageVersion,
+                              projectPart.languageExtension);
+        m_progressCounter.addProgress(1);
+    }
+
+    m_pchTaskQueue.addProjectPchTasks(std::move(pchTasks));
+}
+
 } // namespace ClangBackEnd

src/tools/clangpchmanagerbackend/source/pchtaskgenerator.h

@@ -35,6 +35,7 @@ namespace ClangBackEnd {
 class PchTasksMergerInterface;
 
 class BuildDependenciesProviderInterface;
+class PchTaskQueueInterface;
 class ProgressCounter;
 
 class PchTaskGenerator : public PchTaskGeneratorInterface
@@ -42,21 +43,26 @@ class PchTaskGenerator : public PchTaskGeneratorInterface
 public:
     PchTaskGenerator(BuildDependenciesProviderInterface &buildDependenciesProvider,
                      PchTasksMergerInterface &pchTasksMergerInterface,
-                     ProgressCounter &progressCounter)
+                     ProgressCounter &progressCounter,
+                     PchTaskQueueInterface &pchTaskQueue)
         : m_buildDependenciesProvider(buildDependenciesProvider)
         , m_pchTasksMergerInterface(pchTasksMergerInterface)
         , m_progressCounter(progressCounter)
+        , m_pchTaskQueue(pchTaskQueue)
 
     {}
 
     void addProjectParts(ProjectPartContainers &&projectParts,
                          Utils::SmallStringVector &&toolChainArguments) override;
     void removeProjectParts(const ProjectPartIds &projectsPartIds) override;
+    void addNonSystemProjectParts(ProjectPartContainers &&projectParts,
+                                  Utils::SmallStringVector &&toolChainArguments) override;
 
 private:
     BuildDependenciesProviderInterface &m_buildDependenciesProvider;
     PchTasksMergerInterface &m_pchTasksMergerInterface;
     ProgressCounter &m_progressCounter;
+    PchTaskQueueInterface &m_pchTaskQueue;
 };
 
 } // namespace ClangBackEnd

src/tools/clangpchmanagerbackend/source/pchtaskgeneratorinterface.h

@@ -35,6 +35,9 @@ public:
     virtual void addProjectParts(ProjectPartContainers &&projectParts,
                                  Utils::SmallStringVector &&toolChainArguments)
         = 0;
+    virtual void addNonSystemProjectParts(ProjectPartContainers &&projectParts,
+                                          Utils::SmallStringVector &&toolChainArguments)
+        = 0;
     virtual void removeProjectParts(const ProjectPartIds &projectsPartIds) = 0;
 
 protected:

tests/unit/unittest/mockpchtaskgenerator.h

@@ -35,6 +35,9 @@ public:
     MOCK_METHOD2(addProjectParts,
                  void(const ClangBackEnd::ProjectPartContainers &projectParts,
                       const Utils::SmallStringVector &toolChainArguments));
+    MOCK_METHOD2(addNonSystemProjectParts,
+                 void(const ClangBackEnd::ProjectPartContainers &projectParts,
+                      const Utils::SmallStringVector &toolChainArguments));
     MOCK_METHOD1(removeProjectParts, void(const ClangBackEnd::ProjectPartIds &projectsPartIds));
 
     void addProjectParts(ClangBackEnd::ProjectPartContainers &&projectParts,
@@ -42,4 +45,10 @@ public:
     {
         addProjectParts(projectParts, toolChainArguments);
     }
+
+    void addNonSystemProjectParts(ClangBackEnd::ProjectPartContainers &&projectParts,
+                                  Utils::SmallStringVector &&toolChainArguments) override
+    {
+        addNonSystemProjectParts(projectParts, toolChainArguments);
+    }
 };

tests/unit/unittest/pchmanagerserver-test.cpp

@@ -227,26 +227,36 @@ TEST_F(PchManagerServer, SetPathWatcherNotifier)
 
 TEST_F(PchManagerServer, UpdateProjectPartQueueByPathIds)
 {
+    InSequence s;
     server.updateProjectParts(ClangBackEnd::UpdateProjectPartsMessage{
         {projectPart1, projectPart2, projectPart3, projectPart4, projectPart5, projectPart6},
         {"toolChainArgument"}});
 
     EXPECT_CALL(mockProjectPartsManager,
-                projects(ElementsAre(projectPart1.projectPartId,
+                projects(ElementsAre(projectPart1.projectPartId, projectPart2.projectPartId)))
-                                     projectPart2.projectPartId,
+        .WillOnce(
-                                     projectPart4.projectPartId,
+            Return(std::vector<ClangBackEnd::ProjectPartContainer>{{projectPart1, projectPart2}}));
-                                     projectPart5.projectPartId)))
-        .WillOnce(Return(std::vector<ClangBackEnd::ProjectPartContainer>{
-            {projectPart1, projectPart2, projectPart4, projectPart5}}));
     EXPECT_CALL(mockPchTaskGenerator,
-                addProjectParts(ElementsAre(projectPart1, projectPart2, projectPart4, projectPart5),
+                addProjectParts(ElementsAre(projectPart1, projectPart2),
                                 ElementsAre("toolChainArgument")));
+    EXPECT_CALL(mockProjectPartsManager,
+                projects(ElementsAre(projectPart4.projectPartId, projectPart5.projectPartId)))
+        .WillOnce(
+            Return(std::vector<ClangBackEnd::ProjectPartContainer>{{projectPart4, projectPart5}}));
+    EXPECT_CALL(mockPchTaskGenerator,
+                addNonSystemProjectParts(ElementsAre(projectPart4, projectPart5),
+                                         ElementsAre("toolChainArgument")));
 
-    server.pathsWithIdsChanged({{{projectPartId1, ClangBackEnd::SourceType::TopProjectInclude}, {}},
+    server.pathsWithIdsChanged({{{projectPartId1, ClangBackEnd::SourceType::TopSystemInclude}, {}},
-                                {{projectPartId2, ClangBackEnd::SourceType::TopSystemInclude}, {}},
+                                {{projectPartId1, ClangBackEnd::SourceType::ProjectInclude}, {}},
+                                {{projectPartId1, ClangBackEnd::SourceType::UserInclude}, {}},
+                                {{projectPartId2, ClangBackEnd::SourceType::SystemInclude}, {}},
+                                {{projectPartId2, ClangBackEnd::SourceType::ProjectInclude}, {}},
                                 {{projectPartId3, ClangBackEnd::SourceType::UserInclude}, {}},
-                                {{projectPartId4, ClangBackEnd::SourceType::ProjectInclude}, {}},
+                                {{projectPartId4, ClangBackEnd::SourceType::TopProjectInclude}, {}},
-                                {{projectPartId5, ClangBackEnd::SourceType::SystemInclude}, {}},
+                                {{projectPartId4, ClangBackEnd::SourceType::Source}, {}},
+                                {{projectPartId4, ClangBackEnd::SourceType::UserInclude}, {}},
+                                {{projectPartId5, ClangBackEnd::SourceType::ProjectInclude}, {}},
                                 {{projectPartId6, ClangBackEnd::SourceType::Source}, {}}});
 }
 

tests/unit/unittest/pchtaskgenerator-test.cpp

@@ -26,6 +26,7 @@
 #include "googletest.h"
 
 #include "mockbuilddependenciesprovider.h"
+#include "mockpchtaskqueue.h"
 #include "mockpchtasksmerger.h"
 
 #include <pchtaskgenerator.h>
@@ -52,11 +53,13 @@ class PchTaskGenerator : public testing::Test
 protected:
     NiceMock<MockBuildDependenciesProvider> mockBuildDependenciesProvider;
     NiceMock<MockPchTasksMerger> mockPchTaskMerger;
+    NiceMock<MockPchTaskQueue> mockPchTaskQueue;
     NiceMock<MockFunction<void(int, int)>> mockProgressCounterCallback;
     ClangBackEnd::ProgressCounter progressCounter{mockProgressCounterCallback.AsStdFunction()};
     ClangBackEnd::PchTaskGenerator generator{mockBuildDependenciesProvider,
                                              mockPchTaskMerger,
-                                             progressCounter};
+                                             progressCounter,
+                                             mockPchTaskQueue};
     ClangBackEnd::ProjectPartContainer projectPart1{
         1,
         {"--yi"},
@@ -145,6 +148,36 @@ TEST_F(PchTaskGenerator, AddProjectParts)
     generator.addProjectParts({projectPart1}, {"ToolChainArgument"});
 }
 
+TEST_F(PchTaskGenerator, AddNonSystemProjectParts)
+{
+    ON_CALL(mockBuildDependenciesProvider, create(_)).WillByDefault(Return(buildDependency));
+
+    EXPECT_CALL(
+        mockPchTaskQueue,
+        addProjectPchTasks(ElementsAre(AllOf(
+            Field(&PchTask::projectPartIds, ElementsAre(ProjectPartId{1})),
+            Field(&PchTask::includes, ElementsAre(3)),
+            Field(&PchTask::watchedSystemIncludes, ElementsAre(4, 5)),
+            Field(&PchTask::watchedProjectIncludes, ElementsAre(1, 3)),
+            Field(&PchTask::watchedUserIncludes, ElementsAre(2)),
+            Field(&PchTask::watchedUserSources, ElementsAre(6)),
+            Field(&PchTask::compilerMacros,
+                  ElementsAre(CompilerMacro{"YI", "1", 1}, CompilerMacro{"SAN", "3", 3})),
+            Field(&PchTask::systemIncludeSearchPaths,
+                  ElementsAre(IncludeSearchPath{"/system/path", 2, IncludeSearchPathType::System},
+                              IncludeSearchPath{"/builtin/path", 3, IncludeSearchPathType::BuiltIn},
+                              IncludeSearchPath{"/framework/path", 1, IncludeSearchPathType::System})),
+            Field(&PchTask::projectIncludeSearchPaths,
+                  ElementsAre(IncludeSearchPath{"/to/path1", 1, IncludeSearchPathType::User},
+                              IncludeSearchPath{"/to/path2", 2, IncludeSearchPathType::User})),
+            Field(&PchTask::toolChainArguments, ElementsAre("--yi")),
+            Field(&PchTask::language, Eq(Utils::Language::Cxx)),
+            Field(&PchTask::languageVersion, Eq(Utils::LanguageVersion::CXX11)),
+            Field(&PchTask::languageExtension, Eq(Utils::LanguageExtension::All))))));
+
+    generator.addNonSystemProjectParts({projectPart1}, {"ToolChainArgument"});
+}
+
 TEST_F(PchTaskGenerator, ProgressCounter)
 {
     ON_CALL(mockBuildDependenciesProvider, create(_)).WillByDefault(Return(buildDependency));