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

Reply via email to