[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 &Arg) {
+    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