[PATCH] D127229: [clang][deps] Set -disable-free for module compilations

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added inline comments.


================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:338
+                        // always true for a driver invocation.
+                        bool DisableFree = true;
                         DependencyScanningAction Action(
----------------
jansvoboda11 wrote:
> jansvoboda11 wrote:
> > I see the driver is adding `-disable-free` conditionally:
> > 
> > ```
> >   if (!C.isForDiagnostics())
> >     CmdArgs.push_back("-disable-free");
> > ```
> > 
> > Does that change anything for this patch?
> If this is always `true` for our purposes, is there a reason for passing this argument into `DependencyScanningAction` instead of just hard-coding it there?
`C.isForDiagnostics()` is only true in `Driver::generateCompilationDiagnostics`, which didn't seem relevant to this tool.

> If this is always true for our purposes, is there a reason for passing this argument into DependencyScanningAction instead of just hard-coding it there?

My thinking here was that making this an explicit option forces us to think about it if we add another way to trigger dependency scanning that doesn't use a driver invocation directly - for example, in our downstream branch `experimental/cas/main`, we have another path through this code that starts from a cc1 command-line, in which case I would think we should not add -disable-free unless the original command-line set it, since it's explicit in cc1.  Obviously we don't need to bend over backwards to accommodate out-of-tree code here, but it seemed like a good indication this should be explicit, since it's easy to do that.  WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127229