jansvoboda11 marked an inline comment as done. jansvoboda11 added inline comments.
================ Comment at: clang/lib/Lex/HeaderSearch.cpp:264 + if (Module || !AllowSearch || !HSOpts->ImplicitModuleMaps) { + noteModuleLookupUsage(Module, ImportLoc); return Module; ---------------- jansvoboda11 wrote: > ahoppen wrote: > > Just a thought: Could we move `noteModuleLookupUsage` into `findModule`? > > IIUC that would guarantee that we’re catching all cases and we wouldn’t > > need to call `noteModuleLookupUsage ` from both overloads of `lookupModule`. > That would clean up `HeaderSearch` nicely. I'll look into creating new > `HeaderSearch::findModule` function that would wrap `ModuleMap::findModule` > and note usage. Looking more into this, `ModuleMap::findModule` is used in a lot of different places. Since `noteModuleLookupUsage` takes `SourceLocation` of the `#include`, putting it into `findModule` that doesn't require `SourceLocation` ATM would be somewhat intrusive change. I have cleaned up this patch so that `noteModuleLookupUsage` is called only in single `HeaderSearch::lookupModule`. I think this is good enough. WDYT? ================ Comment at: clang/lib/Lex/ModuleMap.cpp:851 new Module("<private>", Loc, Parent, /*IsFramework*/ false, /*IsExplicit*/ true, NumCreatedModules++); Result->Kind = Module::PrivateModuleFragment; ---------------- jansvoboda11 wrote: > ahoppen wrote: > > Maybe a stupid question but why don’t we need to call the callback e.g. > > here (or on the other `new Module`) calls in this file? > This is related to C++20 modules, so it wasn't necessary for the test case I > had in mind. > > But I agree we should handle this as well. I think more robust solution would > be to add factory function to `ModuleMap` through which all `Module` > instances get created, and invoke the callback there. This is fixed in D116751. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113676/new/ https://reviews.llvm.org/D113676 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits