Addressing code review. Removed the helper class, hence we do not need to manage any states.

qiongsiwu · qiongsiwu · commit b7b27955bddf · 2025-02-12T20:19:39.000-08:00
diff --git a/include/swift/Frontend/ModuleInterfaceLoader.h b/include/swift/Frontend/ModuleInterfaceLoader.h
@@ -628,53 +628,20 @@ struct SwiftInterfaceInfo {
   std::optional<version::Version> CompilerToolsVersion;
 };
 
-/// A small helper class that expands the module name to a full output path
-/// with context hash.
-/// The cannonical way to use this class is the following:
-/// 1. Create an instance through the constructor.
-/// 2. Optional: prune the extra args if necessary.
-/// 3. Obtain the expanded name that contains a full path, and a hash.
-/// expandedName is computed only once, and never updated.
-/// Currently, there is no use case where we first obtain expandedName,
-/// then prune the extra args, and finally obtain expandedName again.
-/// Pruning always happens before the first time we call getExpandedName().
-/// Therefore it is correct to calculate expandedName only once.
-class InterfaceModuleOutputPathResolver {
-public:
-  using ArgListTy = std::vector<std::string>;
-  struct ResultTy {
-    llvm::SmallString<256> outputPath;
-
-    // Hash points to a segment of outputPath.
-    StringRef hash;
-  };
-
-private:
-  const StringRef moduleName;
-  const StringRef interfacePath;
-  const StringRef sdkPath;
-  const CompilerInvocation &CI;
+namespace SwiftInterfaceModuleOutputPathResolution {
+struct ResultTy {
+  llvm::SmallString<256> outputPath;
 
-  // This field contains the clang ExtraArgs that affect how clang types are
-  // imported into swift module. See the constructor for how this field is set,
-  // and see pruneExtraArgs to see how it can be updated.
-  ArgListTy extraArgs;
+  // Hash points to a segment of outputPath.
+  StringRef hash;
+};
 
-  std::unique_ptr<ResultTy> resolvedOutputPath;
+using ArgListTy = std::vector<std::string>;
 
-  std::string getHash();
-
-public:
-  InterfaceModuleOutputPathResolver(const StringRef moduleName,
-                                    const StringRef interfacePath,
-                                    const StringRef sdkPath,
-                                    const CompilerInvocation &CI)
-      : moduleName(moduleName), interfacePath(interfacePath), sdkPath(sdkPath),
-        CI(CI), extraArgs(CI.getClangImporterOptions()
-                              .getReducedExtraArgsForSwiftModuleDependency()) {}
-  ResultTy getOutputPath();
-  void pruneExtraArgs(std::function<void(ArgListTy &)> filter);
-};
+void getOutputPath(ResultTy &outputPath, const StringRef &moduleName,
+                   const StringRef &interfacePath, const StringRef &sdkPath,
+                   const CompilerInvocation &CI, const ArgListTy &extraArgs);
+} // namespace SwiftInterfaceModuleOutputPathResolution
 
 struct InterfaceSubContextDelegateImpl : InterfaceSubContextDelegate {
 private:
@@ -750,11 +717,12 @@ struct InterfaceSubContextDelegateImpl : InterfaceSubContextDelegate {
 
   ~InterfaceSubContextDelegateImpl() = default;
 
-  /// includes a hash of relevant key data.
-  InterfaceModuleOutputPathResolver::ResultTy
-  getCachedOutputPath(StringRef moduleName, StringRef interfacePath,
-                      StringRef sdkPath);
+  /// resolvedOutputPath includes a hash of relevant key data.
+  void getCachedOutputPath(
+      SwiftInterfaceModuleOutputPathResolution::ResultTy &resolvedOutputPath,
+      StringRef moduleName, StringRef interfacePath, StringRef sdkPath);
 };
+
 } // namespace swift
 
 #endif
diff --git a/lib/DependencyScan/ScanDependencies.cpp b/lib/DependencyScan/ScanDependencies.cpp
@@ -88,15 +88,6 @@ class ExplicitModuleDependencyResolver {
         tracker(std::move(tracker)) {
     // Copy commandline.
     commandline = resolvingDepInfo.getCommandline();
-
-    if (resolvingDepInfo.isSwiftInterfaceModule()) {
-      auto swiftTextualDeps = resolvingDepInfo.getAsSwiftInterfaceModule();
-      auto &interfacePath = swiftTextualDeps->swiftInterfaceFile;
-      auto SDKPath = instance.getASTContext().SearchPathOpts.getSDKPath();
-      outputPathResolver = std::make_unique<InterfaceModuleOutputPathResolver>(
-          moduleID.ModuleName, interfacePath, SDKPath,
-          instance.getInvocation());
-    }
   }
 
   llvm::Error
@@ -178,12 +169,10 @@ class ExplicitModuleDependencyResolver {
       }
     }
 
-    // It is necessary to update the command line to take
-    // the pruned unused VFS overlay into account before remapping the
-    // command line.
+    SwiftInterfaceModuleOutputPathResolution::ResultTy swiftInterfaceOutputPath;
     if (resolvingDepInfo.isSwiftInterfaceModule()) {
-      pruneUnusedVFSOverlay();
-      updateSwiftInterfaceModuleOutputPath();
+      pruneUnusedVFSOverlay(swiftInterfaceOutputPath);
+      updateSwiftInterfaceModuleOutputPath(swiftInterfaceOutputPath);
     }
 
     // Update the dependency in the cache with the modified command-line.
@@ -197,7 +186,7 @@ class ExplicitModuleDependencyResolver {
     }
 
     auto dependencyInfoCopy = resolvingDepInfo;
-    if (auto err = finalize(dependencyInfoCopy))
+    if (auto err = finalize(dependencyInfoCopy, swiftInterfaceOutputPath))
       return err;
 
     dependencyInfoCopy.setIsFinalized(true);
@@ -207,15 +196,16 @@ class ExplicitModuleDependencyResolver {
 
 private:
   // Finalize the resolving dependency info.
-  llvm::Error finalize(ModuleDependencyInfo &depInfo) {
+  llvm::Error finalize(ModuleDependencyInfo &depInfo,
+                       const SwiftInterfaceModuleOutputPathResolution::ResultTy
+                           &swiftInterfaceModuleOutputPath) {
     if (resolvingDepInfo.isSwiftPlaceholderModule())
       return llvm::Error::success();
 
-    if (outputPathResolver) {
-      auto resolvedPath = outputPathResolver->getOutputPath();
-      depInfo.setOutputPathAndHash(resolvedPath.outputPath.str().str(),
-                                   resolvedPath.hash.str());
-    }
+    if (resolvingDepInfo.isSwiftInterfaceModule())
+      depInfo.setOutputPathAndHash(
+          swiftInterfaceModuleOutputPath.outputPath.str().str(),
+          swiftInterfaceModuleOutputPath.hash.str());
 
     // Add macros.
     for (auto &macro : macros)
@@ -396,16 +386,18 @@ class ExplicitModuleDependencyResolver {
     }
   }
 
-  void pruneUnusedVFSOverlay() {
+  void pruneUnusedVFSOverlay(
+      SwiftInterfaceModuleOutputPathResolution::ResultTy &outputPath) {
     // Pruning of unused VFS overlay options for Clang dependencies is performed
     // by the Clang dependency scanner.
     if (moduleID.Kind == ModuleDependencyKind::Clang)
       return;
 
+    // Prune the command line.
     std::vector<std::string> resolvedCommandLine;
     size_t skip = 0;
-    for (auto it = commandline.begin(), end = commandline.end();
-         it != end; it++) {
+    for (auto it = commandline.begin(), end = commandline.end(); it != end;
+         it++) {
       if (skip) {
         skip--;
         continue;
@@ -423,50 +415,50 @@ class ExplicitModuleDependencyResolver {
       resolvedCommandLine.push_back(*it);
     }
 
-    // Filter the extra args that we use to calculate the hash of the module.
-    // The key assumption is that these args are clang importer args, so we
-    // do not need to check for -Xcc.
-    auto extraArgsFilter =
-        [&](InterfaceModuleOutputPathResolver::ArgListTy &args) {
-          bool skip = false;
-          InterfaceModuleOutputPathResolver::ArgListTy newArgs;
-          for (auto it = args.begin(), end = args.end(); it != end; it++) {
-            if (skip) {
-              skip = false;
-              continue;
-            }
-
-            if ((it + 1) != end && isVFSOverlayFlag(*it)) {
-              if (!usedVFSOverlayPaths.contains(*(it + 1))) {
-                skip = true;
-                continue;
-              }
-            }
-
-            newArgs.push_back(*it);
-          }
-          args = std::move(newArgs);
-          return;
-        };
+    commandline = std::move(resolvedCommandLine);
 
-    if (outputPathResolver)
-      outputPathResolver->pruneExtraArgs(extraArgsFilter);
+    // Prune the clang impoter options. We do not need to deal with -Xcc because
+    // these are clang options.
+    const auto &CI = instance.getInvocation();
 
-    commandline = std::move(resolvedCommandLine);
-  }
+    SwiftInterfaceModuleOutputPathResolution::ArgListTy extraArgsList;
+    const SwiftInterfaceModuleOutputPathResolution::ArgListTy
+        &clangImpporterOptions =
+            CI.getClangImporterOptions()
+                .getReducedExtraArgsForSwiftModuleDependency();
 
-  void updateSwiftInterfaceModuleOutputPath() {
-    // The command line needs update once we prune the unused VFS overlays.
-    // The update consists of two steps.
-    // 1. Obtain the output path, which includes the module hash that takes the
-    // VFS pruning into account.
-    // 2. Update `-o `'s value on the command line with the new output path.
+    skip = 0;
+    for (auto it = clangImpporterOptions.begin(),
+              end = clangImpporterOptions.end();
+         it != end; it++) {
+      if (skip) {
+        skip = 0;
+        continue;
+      }
+
+      if ((it + 1) != end && isVFSOverlayFlag(*it)) {
+        if (!usedVFSOverlayPaths.contains(*(it + 1))) {
+          skip = 1;
+          continue;
+        }
+      }
 
-    assert(outputPathResolver &&
-           "Can only update if we have an outputPathResolver.");
-    const auto &expandedName = outputPathResolver->getOutputPath();
+      extraArgsList.push_back(*it);
+    }
+
+    auto swiftTextualDeps = resolvingDepInfo.getAsSwiftInterfaceModule();
+    auto &interfacePath = swiftTextualDeps->swiftInterfaceFile;
+    auto sdkPath = instance.getASTContext().SearchPathOpts.getSDKPath();
+    SwiftInterfaceModuleOutputPathResolution::getOutputPath(
+        outputPath, moduleID.ModuleName, interfacePath, sdkPath, CI,
+        extraArgsList);
+
+    return;
+  }
 
-    StringRef outputName = expandedName.outputPath.str();
+  void updateSwiftInterfaceModuleOutputPath(
+      const SwiftInterfaceModuleOutputPathResolution::ResultTy &outputPath) {
+    StringRef outputName = outputPath.outputPath.str();
 
     bool isOutputPath = false;
     for (auto &A : commandline) {
@@ -604,7 +596,6 @@ class ExplicitModuleDependencyResolver {
   ModuleDependenciesCache &cache;
   CompilerInstance &instance;
   const ModuleDependencyInfo &resolvingDepInfo;
-  std::unique_ptr<InterfaceModuleOutputPathResolver> outputPathResolver;
 
   std::optional<SwiftDependencyTracker> tracker;
   std::vector<std::string> rootIDs;
diff --git a/lib/Frontend/ModuleInterfaceLoader.cpp b/lib/Frontend/ModuleInterfaceLoader.cpp
@@ -1098,8 +1098,10 @@ class ModuleInterfaceLoaderImpl {
         requiresOSSAModules);
 
     // Compute the output path if we're loading or emitting a cached module.
-    auto resolvedOutputPath = astDelegate.getCachedOutputPath(
-        moduleName, interfacePath, ctx.SearchPathOpts.getSDKPath());
+    SwiftInterfaceModuleOutputPathResolution::ResultTy resolvedOutputPath;
+    astDelegate.getCachedOutputPath(resolvedOutputPath, moduleName,
+                                    interfacePath,
+                                    ctx.SearchPathOpts.getSDKPath());
     auto &cachedOutputPath = resolvedOutputPath.outputPath;
 
     // Try to find the right module for this interface, either alongside it,
@@ -2005,13 +2007,14 @@ InterfaceSubContextDelegateImpl::InterfaceSubContextDelegateImpl(
 
 /// Calculate an output filename in \p genericSubInvocation's cache path that
 /// includes a hash of relevant key data.
-InterfaceModuleOutputPathResolver::ResultTy
-InterfaceSubContextDelegateImpl::getCachedOutputPath(StringRef moduleName,
-                                                     StringRef interfacePath,
-                                                     StringRef sdkPath) {
-  InterfaceModuleOutputPathResolver resolver(moduleName, interfacePath, sdkPath,
-                                             genericSubInvocation);
-  return resolver.getOutputPath();
+void InterfaceSubContextDelegateImpl::getCachedOutputPath(
+    SwiftInterfaceModuleOutputPathResolution::ResultTy &resolvedOutputPath,
+    StringRef moduleName, StringRef interfacePath, StringRef sdkPath) {
+  SwiftInterfaceModuleOutputPathResolution::getOutputPath(
+      resolvedOutputPath, moduleName, interfacePath, sdkPath,
+      genericSubInvocation,
+      genericSubInvocation.getClangImporterOptions()
+          .getReducedExtraArgsForSwiftModuleDependency());
 }
 
 std::error_code
@@ -2090,8 +2093,8 @@ InterfaceSubContextDelegateImpl::runInSubCompilerInstance(StringRef moduleName,
   }
 
   // Calculate output path of the module.
-  auto resolvedOutputPath =
-      getCachedOutputPath(moduleName, interfacePath, sdkPath);
+  SwiftInterfaceModuleOutputPathResolution::ResultTy resolvedOutputPath;
+  getCachedOutputPath(resolvedOutputPath, moduleName, interfacePath, sdkPath);
 
   // If no specific output path is given, use the hashed output path.
   if (outputPath.empty()) {
@@ -2744,24 +2747,7 @@ std::unique_ptr<ExplicitCASModuleLoader> ExplicitCASModuleLoader::create(
   return result;
 }
 
-InterfaceModuleOutputPathResolver::ResultTy
-InterfaceModuleOutputPathResolver::getOutputPath() {
-  if (!resolvedOutputPath) {
-    resolvedOutputPath = std::make_unique<ResultTy>();
-    auto &outputPath = resolvedOutputPath->outputPath;
-    outputPath = CI.getClangModuleCachePath();
-    llvm::sys::path::append(outputPath, moduleName);
-    outputPath.append("-");
-    auto hashStart = outputPath.size();
-    outputPath.append(getHash());
-    resolvedOutputPath->hash = outputPath.str().substr(hashStart);
-    outputPath.append(".");
-    auto outExt = file_types::getExtension(file_types::TY_SwiftModuleFile);
-    outputPath.append(outExt);
-  }
-  return *resolvedOutputPath;
-}
-
+namespace swift::SwiftInterfaceModuleOutputPathResolution {
 /// Construct a key for the .swiftmodule being generated. There is a
 /// balance to be struck here between things that go in the cache key and
 /// things that go in the "up to date" check of the cache entry. We want to
@@ -2772,7 +2758,10 @@ InterfaceModuleOutputPathResolver::getOutputPath() {
 /// -- rather than making a new one and potentially filling up the cache
 /// with dead entries -- when other factors change, such as the contents of
 /// the .swiftinterface input or its dependencies.
-std::string InterfaceModuleOutputPathResolver::getHash() {
+static std::string getContextHash(const CompilerInvocation &CI,
+                                  const StringRef &interfacePath,
+                                  const StringRef &sdkPath,
+                                  const ArgListTy &extraArgs) {
   // When doing dependency scanning for explicit module, use strict context hash
   // to ensure sound module hash.
   bool useStrictCacheHash = CI.getFrontendOptions().RequestedAction ==
@@ -2842,9 +2831,19 @@ std::string InterfaceModuleOutputPathResolver::getHash() {
   return llvm::toString(llvm::APInt(64, H), 36, /*Signed=*/false);
 }
 
-void InterfaceModuleOutputPathResolver::pruneExtraArgs(
-    std::function<void(ArgListTy &)> filter) {
-  assert(!resolvedOutputPath &&
-         "Cannot prune again after the output path has been resolved.");
-  filter(extraArgs);
+void getOutputPath(ResultTy &resolvedOutputPath, const StringRef &moduleName,
+                   const StringRef &interfacePath, const StringRef &sdkPath,
+                   const CompilerInvocation &CI, const ArgListTy &extraArgs) {
+  auto &outputPath = resolvedOutputPath.outputPath;
+  outputPath = CI.getClangModuleCachePath();
+  llvm::sys::path::append(outputPath, moduleName);
+  outputPath.append("-");
+  auto hashStart = outputPath.size();
+  outputPath.append(getContextHash(CI, interfacePath, sdkPath, extraArgs));
+  resolvedOutputPath.hash = outputPath.str().substr(hashStart);
+  outputPath.append(".");
+  auto outExt = file_types::getExtension(file_types::TY_SwiftModuleFile);
+  outputPath.append(outExt);
+  return;
 }
+} // namespace swift::SwiftInterfaceModuleOutputPathResolution
