benlangmuir updated this revision to Diff 435237.
benlangmuir added a comment.
Removed use of std::unique_ptr in DependencyScanningTool.cpp, per review
feedback.
@jansvoboda11 Note: there is another map containing
`std::unique_ptr<ModuleDeps>` in `ModuleDepCollector`, but that one is required
for correctness. The problem is that there is code that forms a `ModuleDeps &`
lvalue and assumes it will be correct after mutating the containing
`ModularDeps` map. For example, the signature of `addAllSubmoduleDeps`
requires this. If we want to switch to using this as a value in the map, we
would need to do the lookup in the map again after each mutation. This code
worked previously with `unordered_map`, because it guarantees that keys and
values are not mutated by unrelated mutations of the containing map, unlike
`MapVector`. In practice, this means there is no additional overhead from the
unique_ptr, because it was already doing a separate allocation for each value
in the unordered_map.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127243/new/
https://reviews.llvm.org/D127243
Files:
clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===================================================================
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -199,7 +199,7 @@
MDC.Consumer.handleDependencyOutputOpts(*MDC.Opts);
for (auto &&I : MDC.ModularDeps)
- MDC.Consumer.handleModuleDependency(I.second);
+ MDC.Consumer.handleModuleDependency(*I.second);
for (auto &&I : MDC.FileDeps)
MDC.Consumer.handleFileDependency(I);
@@ -212,11 +212,12 @@
assert(M == M->getTopLevelModule() && "Expected top level module!");
// If this module has been handled already, just return its ID.
- auto ModI = MDC.ModularDeps.insert({M, ModuleDeps{}});
+ auto ModI = MDC.ModularDeps.insert({M, nullptr});
if (!ModI.second)
- return ModI.first->second.ID;
+ return ModI.first->second->ID;
- ModuleDeps &MD = ModI.first->second;
+ ModI.first->second = std::make_unique<ModuleDeps>();
+ ModuleDeps &MD = *ModI.first->second;
MD.ID.ModuleName = M->getFullModuleName();
MD.ImportedByMainFile = DirectModularDeps.contains(M);
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
===================================================================
--- clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
@@ -176,7 +176,7 @@
private:
std::vector<std::string> Dependencies;
std::vector<PrebuiltModuleDep> PrebuiltModuleDeps;
- std::map<std::string, ModuleDeps> ClangModuleDeps;
+ llvm::MapVector<std::string, ModuleDeps, llvm::StringMap<unsigned>>
ClangModuleDeps;
std::string ContextHash;
std::vector<std::string> OutputPaths;
const llvm::StringSet<> &AlreadySeen;
Index: clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
===================================================================
--- clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
+++ clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
@@ -149,9 +149,9 @@
/// The parent dependency collector.
ModuleDepCollector &MDC;
/// Working set of direct modular dependencies.
- llvm::DenseSet<const Module *> DirectModularDeps;
+ llvm::SetVector<const Module *> DirectModularDeps;
/// Working set of direct modular dependencies that have already been built.
- llvm::DenseSet<const Module *> DirectPrebuiltModularDeps;
+ llvm::SetVector<const Module *> DirectPrebuiltModularDeps;
void handleImport(const Module *Imported);
@@ -199,7 +199,7 @@
/// textually included header files.
std::vector<std::string> FileDeps;
/// Direct and transitive modular dependencies of the main source file.
- std::unordered_map<const Module *, ModuleDeps> ModularDeps;
+ llvm::MapVector<const Module *, std::unique_ptr<ModuleDeps>> ModularDeps;
/// Options that control the dependency output generation.
std::unique_ptr<DependencyOutputOptions> Opts;
/// The original Clang invocation passed to dependency scanner.
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===================================================================
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -199,7 +199,7 @@
MDC.Consumer.handleDependencyOutputOpts(*MDC.Opts);
for (auto &&I : MDC.ModularDeps)
- MDC.Consumer.handleModuleDependency(I.second);
+ MDC.Consumer.handleModuleDependency(*I.second);
for (auto &&I : MDC.FileDeps)
MDC.Consumer.handleFileDependency(I);
@@ -212,11 +212,12 @@
assert(M == M->getTopLevelModule() && "Expected top level module!");
// If this module has been handled already, just return its ID.
- auto ModI = MDC.ModularDeps.insert({M, ModuleDeps{}});
+ auto ModI = MDC.ModularDeps.insert({M, nullptr});
if (!ModI.second)
- return ModI.first->second.ID;
+ return ModI.first->second->ID;
- ModuleDeps &MD = ModI.first->second;
+ ModI.first->second = std::make_unique<ModuleDeps>();
+ ModuleDeps &MD = *ModI.first->second;
MD.ID.ModuleName = M->getFullModuleName();
MD.ImportedByMainFile = DirectModularDeps.contains(M);
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
===================================================================
--- clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
@@ -176,7 +176,7 @@
private:
std::vector<std::string> Dependencies;
std::vector<PrebuiltModuleDep> PrebuiltModuleDeps;
- std::map<std::string, ModuleDeps> ClangModuleDeps;
+ llvm::MapVector<std::string, ModuleDeps, llvm::StringMap<unsigned>> ClangModuleDeps;
std::string ContextHash;
std::vector<std::string> OutputPaths;
const llvm::StringSet<> &AlreadySeen;
Index: clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
===================================================================
--- clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
+++ clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h
@@ -149,9 +149,9 @@
/// The parent dependency collector.
ModuleDepCollector &MDC;
/// Working set of direct modular dependencies.
- llvm::DenseSet<const Module *> DirectModularDeps;
+ llvm::SetVector<const Module *> DirectModularDeps;
/// Working set of direct modular dependencies that have already been built.
- llvm::DenseSet<const Module *> DirectPrebuiltModularDeps;
+ llvm::SetVector<const Module *> DirectPrebuiltModularDeps;
void handleImport(const Module *Imported);
@@ -199,7 +199,7 @@
/// textually included header files.
std::vector<std::string> FileDeps;
/// Direct and transitive modular dependencies of the main source file.
- std::unordered_map<const Module *, ModuleDeps> ModularDeps;
+ llvm::MapVector<const Module *, std::unique_ptr<ModuleDeps>> ModularDeps;
/// Options that control the dependency output generation.
std::unique_ptr<DependencyOutputOptions> Opts;
/// The original Clang invocation passed to dependency scanner.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
