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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
