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