Richard, Thanks for looking at this. I'll rework it.
> We should probably only do this if we got to the end of typo-correction and > found no corrections. I'm not so sure about this. For example, in my test case, it would suggest "use_this2" as a typo fixit for "user_this1", instead of finding the symbol in the missing module import. I guess it's a question of whether an exact match should take precedent over a partial match. What do you think? I'll follow your lead. > It looks like this will build every module described in any module map we've > loaded. That could be extremely time-consuming, and could produce errors. Is > that what you want here? Yeah, that's a key question. The problem is that the global module index only has symbols for modules that clang with the current argument hash has built modules before, meaning that if someone forgets an import, they won't see this fixit unless they've imported the module in some other compile with the same hash. So the key options are: 1. Don't build a full global module index. User only sees the fixit for modules already in the global module index. 2. When a symbol can't be resolved, create the full global module index on demand. (My current implementation.) 3. Build a full global module index the first time or whenever the module.map file or headers change. Might be a slow build when this triggers. What do you think? Thanks. -John -----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of Richard Smith Sent: Wednesday, March 26, 2014 3:25 PM To: [email protected]; [email protected]; [email protected] Cc: [email protected] Subject: Re: [PATCH] preview patch for fixit for finding modules needing import/inclusion This looks like basically the right approach to me. ================ Comment at: lib/Sema/SemaLookup.cpp:4060-4061 @@ -4056,1 +4059,4 @@ + // Look for the symbol in non-imported modules. + if (getLangOpts().Modules && getLangOpts().ModulesSearchAll) { + // Try to find the type in modules via the global module index. ---------------- We should probably only do this if we got to the end of typo-correction and found no corrections. ================ Comment at: lib/Sema/SemaLookup.cpp:4096-4098 @@ +4095,5 @@ + TC.setCorrectionDecl(ModRes.getAsSingle<NamedDecl>()); + // Hide the module again. diagnoseTypo will unhide the decl module. + Loader.makeModuleVisible(TheModule, Module::Hidden, + TypoName.getLoc(), false); + } ---------------- This won't work. We don't support visibility decreasing. Instead, you should be able to just load the module rather than making it visible, and tell your `LookupResult` to find hidden results during the lookup. ================ Comment at: lib/Frontend/CompilerInstance.cpp:1440 @@ +1439,3 @@ + // Load a module as hidden. This also adds it to the global index. + ModuleLoadResult Result = loadModule(TheModule->DefinitionLoc, Path, + Module::Hidden, false); ---------------- It looks like this will build every module described in any module map we've loaded. That could be extremely time-consuming, and could produce errors. Is that what you want here? ================ Comment at: lib/Serialization/GlobalModuleIndex.cpp:345-358 @@ -344,1 +344,16 @@ +void GlobalModuleIndex::dump() { + std::fprintf(stderr, "*** Global Module Index Dump:\n"); + std::fprintf(stderr, "Module files:\n"); + for (llvm::SmallVector<ModuleInfo, 16>::iterator I = Modules.begin(), + E = Modules.end(); I != E; ++I) { + ModuleInfo *MI = (ModuleInfo*)I; + std::fprintf(stderr, "** %s\n", MI->FileName.c_str()); + if (MI->File) + MI->File->dump(); + else + std::fprintf(stderr, "\n"); + } + std::fprintf(stderr, "\n"); +} + ---------------- Feel free to just check this part in. http://llvm-reviews.chandlerc.com/D2671 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
