jansvoboda11 added a comment.
Thanks for the feedback, I'll try to clean this up a bit.
================
Comment at: clang/lib/Lex/HeaderSearch.cpp:94
+ // Module map parsing initiated by header search.
+ if (HS.CurrentSearchPathIdx != ~0U)
+ HS.ModuleToSearchDirIdx[M] = HS.CurrentSearchPathIdx;
----------------
ahoppen wrote:
> When would the `moduleMapModuleCreated` be called while `CurrentSearchPathIdx
> == ~0U`? Could this be an `assert` instead?
This happens whenever any of the `ModuleMap` member functions that create new
`Module` instances are called outside of `HeaderSearch`.
The `MMCallback` callback is basically "global" (present for the whole lifetime
of `ModuleMap`), so that we don't have to repeatedly register/deregister it in
`HeaderSearch::lookupModule`.
================
Comment at: clang/lib/Lex/HeaderSearch.cpp:264
+ if (Module || !AllowSearch || !HSOpts->ImplicitModuleMaps) {
+ noteModuleLookupUsage(Module, ImportLoc);
return Module;
----------------
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.
================
Comment at: clang/lib/Lex/ModuleMap.cpp:851
new Module("<private>", Loc, Parent, /*IsFramework*/ false,
/*IsExplicit*/ true, NumCreatedModules++);
Result->Kind = Module::PrivateModuleFragment;
----------------
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.
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
