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

[Jan Svoboda via Phabricator via cfe-commits]

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

LGTM!



================
Comment at: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp:338
+                        // always true for a driver invocation.
+                        bool DisableFree = true;
                         DependencyScanningAction Action(
----------------
benlangmuir wrote:
> 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?
That makes sense, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127229