jansvoboda11 added inline comments.
================ Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:56-58 + // Ensure the reported context hash is strict. + CI.getHeaderSearchOpts().ModulesStrictContextHash = true; + ---------------- dexonsmith wrote: > IIUC, explicit modules don't really have/need a context hash. Can related > options be stripped out when serializing to `-cc1` when `ImplicitModules` is > false? > > Basically, I'm asking if `ModulesStrictContextHash` is a no-op when > `ImplicitModules` is false. If not, can we make it a no-op? > (If we can, then maybe rename the field to `ImplicitModulesStrictContextHash` > and audit that no one reads it when `ImplicitModules` is off...) Let me clarify this a bit. You're right that when building explicit modules, we don't care about context hash. We do care about using **strict** context hash during the scan though - it's an implementation detail through which we prevent mixing incompatible modules/TUs. (This strict context hash is enabled elsewhere in the dependency scanner.) At the end of the scan, we take discovered modules and modify/prune their `CompilerInvocation` (in this function). This can essentially "merge" multiple versions of the same module into one, which is very desirable. But we still want to do it according to the **strict** context hash. We don't want to merge versions with different search paths for example (non-strict context hash). That's what this change ensures. Note that we don't **need** to report context hashes to scanner clients. Any other identifier derived from a strict context hash would work. ================ Comment at: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp:56-58 + // Ensure the reported context hash is strict. + CI.getHeaderSearchOpts().ModulesStrictContextHash = true; + ---------------- jansvoboda11 wrote: > dexonsmith wrote: > > IIUC, explicit modules don't really have/need a context hash. Can related > > options be stripped out when serializing to `-cc1` when `ImplicitModules` > > is false? > > > > Basically, I'm asking if `ModulesStrictContextHash` is a no-op when > > `ImplicitModules` is false. If not, can we make it a no-op? > > (If we can, then maybe rename the field to > > `ImplicitModulesStrictContextHash` and audit that no one reads it when > > `ImplicitModules` is off...) > Let me clarify this a bit. You're right that when building explicit modules, > we don't care about context hash. > > We do care about using **strict** context hash during the scan though - it's > an implementation detail through which we prevent mixing incompatible > modules/TUs. (This strict context hash is enabled elsewhere in the dependency > scanner.) > > At the end of the scan, we take discovered modules and modify/prune their > `CompilerInvocation` (in this function). This can essentially "merge" > multiple versions of the same module into one, which is very desirable. But > we still want to do it according to the **strict** context hash. We don't > want to merge versions with different search paths for example (non-strict > context hash). That's what this change ensures. > > Note that we don't **need** to report context hashes to scanner clients. Any > other identifier derived from a strict context hash would work. I think the rename you're suggesting is valid. We //could// strip the `ModulesStrictContextHash` in the scanner: after we generate the strict context hash and before we generate the command-line. I think that can be done in a NFC follow-up. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111720/new/ https://reviews.llvm.org/D111720 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
