[PATCH] D120474: [clang][deps] Remove '-fmodules-cache-path=' arguments

[Jan Svoboda via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcf4a31fc0f97: [clang][deps] Remove 
-fmodules-cache-path= arguments (authored by jansvoboda11).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120474/new/

https://reviews.llvm.org/D120474

Files:
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/modules-inferred-explicit-build.m
  clang/tools/clang-scan-deps/ClangScanDeps.cpp


Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -374,11 +374,11 @@
 const ModuleDeps  = MDIt->second;
 
 StringRef Filename = llvm::sys::path::filename(MD.ImplicitModulePCMPath);
+StringRef ModuleCachePath = llvm::sys::path::parent_path(
+llvm::sys::path::parent_path(MD.ImplicitModulePCMPath));
 
-SmallString<256> ExplicitPCMPath(
-!ModuleFilesDir.empty()
-? ModuleFilesDir
-: MD.BuildInvocation.getHeaderSearchOpts().ModuleCachePath);
+SmallString<256> ExplicitPCMPath(!ModuleFilesDir.empty() ? ModuleFilesDir
+ : 
ModuleCachePath);
 llvm::sys::path::append(ExplicitPCMPath, MD.ID.ContextHash, Filename);
 return std::string(ExplicitPCMPath);
   }
Index: clang/test/ClangScanDeps/modules-inferred-explicit-build.m
===
--- clang/test/ClangScanDeps/modules-inferred-explicit-build.m
+++ clang/test/ClangScanDeps/modules-inferred-explicit-build.m
@@ -12,7 +12,7 @@
 // RUN: %python %S/../../utils/module-deps-to-rsp.py %t.db --tu-index=0 > 
%t.tu.rsp
 // RUN: %clang @%t.inferred.cc1.rsp -pedantic -Werror
 // RUN: %clang @%t.system.cc1.rsp -pedantic -Werror
-// RUN: %clang @%t.tu.rsp -pedantic -Werror -Wno-unused-command-line-argument
+// RUN: %clang @%t.tu.rsp -pedantic -Werror
 
 #include 
 #include 
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -51,6 +51,7 @@
 
   CI.getLangOpts()->ImplicitModules = false;
   CI.getHeaderSearchOpts().ImplicitModuleMaps = false;
+  CI.getHeaderSearchOpts().ModuleCachePath.clear();
 
   // Report the prebuilt modules this module uses.
   for (const auto  : Deps.PrebuiltModuleDeps)
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
@@ -31,7 +31,13 @@
   getAdditionalArgsWithoutModulePaths();
   Args.insert(Args.end(), AdditionalArgs.begin(), AdditionalArgs.end());
 
-  // TODO: Filter out implicit modules leftovers (e.g. 
"-fmodules-cache-path=").
+  // This argument is unused in explicit compiles.
+  llvm::erase_if(Args, [](const std::string ) {
+return Arg.find("-fmodules-cache-path=") == 0;
+  });
+
+  // TODO: Filter out the remaining implicit modules leftovers
+  // (e.g. "-fmodules-prune-interval=" or "-fmodules-prune-after=").
 
   return Args;
 }


Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -374,11 +374,11 @@
 const ModuleDeps  = MDIt->second;
 
 StringRef Filename = llvm::sys::path::filename(MD.ImplicitModulePCMPath);
+StringRef ModuleCachePath = llvm::sys::path::parent_path(
+llvm::sys::path::parent_path(MD.ImplicitModulePCMPath));
 
-SmallString<256> ExplicitPCMPath(
-!ModuleFilesDir.empty()
-? ModuleFilesDir
-: MD.BuildInvocation.getHeaderSearchOpts().ModuleCachePath);
+SmallString<256> ExplicitPCMPath(!ModuleFilesDir.empty() ? ModuleFilesDir
+ : ModuleCachePath);
 llvm::sys::path::append(ExplicitPCMPath, MD.ID.ContextHash, Filename);
 return std::string(ExplicitPCMPath);
   }
Index: clang/test/ClangScanDeps/modules-inferred-explicit-build.m
===
--- clang/test/ClangScanDeps/modules-inferred-explicit-build.m
+++ clang/test/ClangScanDeps/modules-inferred-explicit-build.m
@@ -12,7 +12,7 @@
 // RUN: %python %S/../../utils/module-deps-to-rsp.py %t.db --tu-index=0 > %t.tu.rsp
 // RUN: %clang @%t.inferred.cc1.rsp -pedantic -Werror
 // RUN: %clang @%t.system.cc1.rsp -pedantic -Werror
-// RUN: 

[PATCH] D120474: [clang][deps] Remove '-fmodules-cache-path=' arguments

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120474/new/

https://reviews.llvm.org/D120474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120474: [clang][deps] Remove '-fmodules-cache-path=' arguments

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 413387.
jansvoboda11 added a comment.

Update TODO instead of removing it


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120474/new/

https://reviews.llvm.org/D120474

Files:
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/modules-inferred-explicit-build.m
  clang/tools/clang-scan-deps/ClangScanDeps.cpp


Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -374,11 +374,11 @@
 const ModuleDeps  = MDIt->second;
 
 StringRef Filename = llvm::sys::path::filename(MD.ImplicitModulePCMPath);
+StringRef ModuleCachePath = llvm::sys::path::parent_path(
+llvm::sys::path::parent_path(MD.ImplicitModulePCMPath));
 
-SmallString<256> ExplicitPCMPath(
-!ModuleFilesDir.empty()
-? ModuleFilesDir
-: MD.BuildInvocation.getHeaderSearchOpts().ModuleCachePath);
+SmallString<256> ExplicitPCMPath(!ModuleFilesDir.empty() ? ModuleFilesDir
+ : 
ModuleCachePath);
 llvm::sys::path::append(ExplicitPCMPath, MD.ID.ContextHash, Filename);
 return std::string(ExplicitPCMPath);
   }
Index: clang/test/ClangScanDeps/modules-inferred-explicit-build.m
===
--- clang/test/ClangScanDeps/modules-inferred-explicit-build.m
+++ clang/test/ClangScanDeps/modules-inferred-explicit-build.m
@@ -12,7 +12,7 @@
 // RUN: %python %S/../../utils/module-deps-to-rsp.py %t.db --tu-index=0 > 
%t.tu.rsp
 // RUN: %clang @%t.inferred.cc1.rsp -pedantic -Werror
 // RUN: %clang @%t.system.cc1.rsp -pedantic -Werror
-// RUN: %clang @%t.tu.rsp -pedantic -Werror -Wno-unused-command-line-argument
+// RUN: %clang @%t.tu.rsp -pedantic -Werror
 
 #include 
 #include 
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -51,6 +51,7 @@
 
   CI.getLangOpts()->ImplicitModules = false;
   CI.getHeaderSearchOpts().ImplicitModuleMaps = false;
+  CI.getHeaderSearchOpts().ModuleCachePath.clear();
 
   // Report the prebuilt modules this module uses.
   for (const auto  : Deps.PrebuiltModuleDeps)
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
@@ -31,7 +31,13 @@
   getAdditionalArgsWithoutModulePaths();
   Args.insert(Args.end(), AdditionalArgs.begin(), AdditionalArgs.end());
 
-  // TODO: Filter out implicit modules leftovers (e.g. 
"-fmodules-cache-path=").
+  // This argument is unused in explicit compiles.
+  llvm::erase_if(Args, [](const std::string ) {
+return Arg.find("-fmodules-cache-path=") == 0;
+  });
+
+  // TODO: Filter out the remaining implicit modules leftovers
+  // (e.g. "-fmodules-prune-interval=" or "-fmodules-prune-after=").
 
   return Args;
 }


Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -374,11 +374,11 @@
 const ModuleDeps  = MDIt->second;
 
 StringRef Filename = llvm::sys::path::filename(MD.ImplicitModulePCMPath);
+StringRef ModuleCachePath = llvm::sys::path::parent_path(
+llvm::sys::path::parent_path(MD.ImplicitModulePCMPath));
 
-SmallString<256> ExplicitPCMPath(
-!ModuleFilesDir.empty()
-? ModuleFilesDir
-: MD.BuildInvocation.getHeaderSearchOpts().ModuleCachePath);
+SmallString<256> ExplicitPCMPath(!ModuleFilesDir.empty() ? ModuleFilesDir
+ : ModuleCachePath);
 llvm::sys::path::append(ExplicitPCMPath, MD.ID.ContextHash, Filename);
 return std::string(ExplicitPCMPath);
   }
Index: clang/test/ClangScanDeps/modules-inferred-explicit-build.m
===
--- clang/test/ClangScanDeps/modules-inferred-explicit-build.m
+++ clang/test/ClangScanDeps/modules-inferred-explicit-build.m
@@ -12,7 +12,7 @@
 // RUN: %python %S/../../utils/module-deps-to-rsp.py %t.db --tu-index=0 > %t.tu.rsp
 // RUN: %clang @%t.inferred.cc1.rsp -pedantic -Werror
 // RUN: %clang @%t.system.cc1.rsp -pedantic -Werror
-// RUN: %clang @%t.tu.rsp -pedantic -Werror -Wno-unused-command-line-argument
+// RUN: %clang @%t.tu.rsp -pedantic -Werror
 
 #include 

[PATCH] D120474: [clang][deps] Remove '-fmodules-cache-path=' arguments

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.



Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp:36
+  llvm::erase_if(Args, [](const std::string ) {
+return Arg.find("-fmodules-cache-path=") == 0;
+  });

dexonsmith wrote:
> Is that the only one to remove? (asking because you dropped the FIXME, which 
> suggested it might only be one example)
> 
> Also, could this just be an assertion, since 
> `makeInvocationForModuleBuildWithoutPaths()` is already clearing things?
> - In other words, why do you need this in two places?
> - And if you do need it in both, does the test cover both, or should there be 
> another test or RUN line for the other code path?
Ah, there are some others (e.g. `-fmodules-prune-interval=`, 
`-fmodules-prune-after=`) so you're right dropping the FIXME might be a bit 
hasty. I'll update the fixme instead of removing it.

This cannot be an assertion unfortunately. 
`makeInvocationForModuleBuildWithoutPaths()` is clearing the modules cache path 
in the **-cc1** command lines for building **modules**. This however, is 
dealing with the **driver** command line for building the original 
**translation unit**, which doesn't go through the `CompilerInvocation` 
machinery at all. We're relying on textual manipulation here at this moment. 
(TU command lines are driver command lines that might expand into multiple 
commands and therefore are not guaranteed to be representable by single 
`CompilerInvocation`.)

The test I touched covers only this change to the driver command line for 
building the TU. The error (`-Wunused-command-line-argument`) I test is only 
produced by the driver. This means the other change (clearing modules cache 
path in -cc1 `CompilerInvocation`) is just a preventive measure, not necessary 
to make the updated test pass and not testable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120474/new/

https://reviews.llvm.org/D120474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120474: [clang][deps] Remove '-fmodules-cache-path=' arguments

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.
Herald added a project: All.



Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp:36
+  llvm::erase_if(Args, [](const std::string ) {
+return Arg.find("-fmodules-cache-path=") == 0;
+  });

Is that the only one to remove? (asking because you dropped the FIXME, which 
suggested it might only be one example)

Also, could this just be an assertion, since 
`makeInvocationForModuleBuildWithoutPaths()` is already clearing things?
- In other words, why do you need this in two places?
- And if you do need it in both, does the test cover both, or should there be 
another test or RUN line for the other code path?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120474/new/

https://reviews.llvm.org/D120474

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120474: [clang][deps] Remove '-fmodules-cache-path=' arguments

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 created this revision.
jansvoboda11 added reviewers: Bigcheese, dexonsmith.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

With explicit modules build, the '-fmodules-cache-path=' argument is unused.

This patch removes the argument to avoid warnings or errors (with '-Werror') 
stemming from that.

Depends on D118915 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120474

Files:
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/modules-inferred-explicit-build.m
  clang/tools/clang-scan-deps/ClangScanDeps.cpp


Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -374,11 +374,11 @@
 const ModuleDeps  = MDIt->second;
 
 StringRef Filename = llvm::sys::path::filename(MD.ImplicitModulePCMPath);
+StringRef ModuleCachePath = llvm::sys::path::parent_path(
+llvm::sys::path::parent_path(MD.ImplicitModulePCMPath));
 
-SmallString<256> ExplicitPCMPath(
-!ModuleFilesDir.empty()
-? ModuleFilesDir
-: MD.BuildInvocation.getHeaderSearchOpts().ModuleCachePath);
+SmallString<256> ExplicitPCMPath(!ModuleFilesDir.empty() ? ModuleFilesDir
+ : 
ModuleCachePath);
 llvm::sys::path::append(ExplicitPCMPath, MD.ID.ContextHash, Filename);
 return std::string(ExplicitPCMPath);
   }
Index: clang/test/ClangScanDeps/modules-inferred-explicit-build.m
===
--- clang/test/ClangScanDeps/modules-inferred-explicit-build.m
+++ clang/test/ClangScanDeps/modules-inferred-explicit-build.m
@@ -12,7 +12,7 @@
 // RUN: %python %S/../../utils/module-deps-to-rsp.py %t.db --tu-index=0 > 
%t.tu.rsp
 // RUN: %clang @%t.inferred.cc1.rsp -pedantic -Werror
 // RUN: %clang @%t.system.cc1.rsp -pedantic -Werror
-// RUN: %clang @%t.tu.rsp -pedantic -Werror -Wno-unused-command-line-argument
+// RUN: %clang @%t.tu.rsp -pedantic -Werror
 
 #include 
 #include 
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -51,6 +51,7 @@
 
   CI.getLangOpts()->ImplicitModules = false;
   CI.getHeaderSearchOpts().ImplicitModuleMaps = false;
+  CI.getHeaderSearchOpts().ModuleCachePath.clear();
 
   // Report the prebuilt modules this module uses.
   for (const auto  : Deps.PrebuiltModuleDeps)
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
@@ -31,7 +31,10 @@
   getAdditionalArgsWithoutModulePaths();
   Args.insert(Args.end(), AdditionalArgs.begin(), AdditionalArgs.end());
 
-  // TODO: Filter out implicit modules leftovers (e.g. 
"-fmodules-cache-path=").
+  // This argument is unused in explicit compiles.
+  llvm::erase_if(Args, [](const std::string ) {
+return Arg.find("-fmodules-cache-path=") == 0;
+  });
 
   return Args;
 }


Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -374,11 +374,11 @@
 const ModuleDeps  = MDIt->second;
 
 StringRef Filename = llvm::sys::path::filename(MD.ImplicitModulePCMPath);
+StringRef ModuleCachePath = llvm::sys::path::parent_path(
+llvm::sys::path::parent_path(MD.ImplicitModulePCMPath));
 
-SmallString<256> ExplicitPCMPath(
-!ModuleFilesDir.empty()
-? ModuleFilesDir
-: MD.BuildInvocation.getHeaderSearchOpts().ModuleCachePath);
+SmallString<256> ExplicitPCMPath(!ModuleFilesDir.empty() ? ModuleFilesDir
+ : ModuleCachePath);
 llvm::sys::path::append(ExplicitPCMPath, MD.ID.ContextHash, Filename);
 return std::string(ExplicitPCMPath);
   }
Index: clang/test/ClangScanDeps/modules-inferred-explicit-build.m
===
--- clang/test/ClangScanDeps/modules-inferred-explicit-build.m
+++ clang/test/ClangScanDeps/modules-inferred-explicit-build.m
@@ -12,7 +12,7 @@
 // RUN: %python %S/../../utils/module-deps-to-rsp.py %t.db --tu-index=0 > %t.tu.rsp
 // RUN: %clang @%t.inferred.cc1.rsp -pedantic -Werror
 // RUN: %clang @%t.system.cc1.rsp -pedantic -Werror
-// RUN: %clang