benlangmuir accepted this revision.
benlangmuir added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:180
-  // incompatible modules (e.g. with differences in search paths).
-  CI.getHeaderSearchOpts().ModulesStrictContextHash = true;
-
----------------
jansvoboda11 wrote:
> benlangmuir wrote:
> > I see we're enabling strict hashing in the scaner itself: 
> > `ScanInstance.getHeaderSearchOpts().ModulesStrictContextHash = true;`, 
> > which makes me think this code was never used to influence the scanner's 
> > implicit build.  If that's true, was this code *already* dead before my 
> > change in D129884?  It's not clear to me what it was doing.
> The line you found enables strict context hashing for the `CompilerInstance` 
> that performs the scanning build. That's still important so that we don't 
> squash multiple module configurations into one minimized PCM.
> 
> The line this patch removes used to control generation of the module context 
> hash we report to the client. This used to be done by taking the original TU 
> command line, tweaking it, and calling `CompilerInvocation::getModuleHash()`. 
> Since we now hash the whole command line that's generated from said 
> `CompilerInvocation`, controlling the behavior of `getModuleHash()` by 
> enabling strict context hash is no longer necessary.
Got it, thanks for explaining!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142416

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to