Hi Richard, I've put in the change for adding an ErrorRecovery flag to correctTypo. I'm currently working on getting the Modules tests to pass.
I've run into a snag with decldef.mm, which is pointing out a problem with my scheme that I thought I'd ask you about, and it also raised another question or two. In the process of building a full global module index, there's some kind of error in building the cxx_irgen_right and a few other modules, from the Inputs/module.map file. My first question is: Are diagnostic message disabled while building modules? I don't get any messages from the module build other than a "could not build module" message. Or perhaps did I not set up something correctly in triggering the module build? Also, I noticed that when building the modules, the Inputs/module.map file was parsed with ModuleMap::parseModuleMapFile for each module build. Could ModuleMap objects be shared? Finally, the problem I referred to earlier is that if a correctTypo call with ErrorRecovery == true occurs during a module build, the loadGlobalModuleIndex function I added will be called in the module build thread, while the previous loadGlobalModuleIndex call that triggered the module build is still active. I'm supposing this in not a good thing, and could even trigger a deadlock via the file locking that is done. What would be the best way to address this? I'm thinking perhaps have some "InGlobalModuleIndexBuild" flag somewhere to be checked in loadGlobalModuleIndex, and return either the existing index (perhaps loading a fresh instance), or return null to trigger correctTypo to skip the missing import check. If this is the way to go, where would I put this flag, or otherwise how would I implement it? The module build has its own CompileInstance object instance, so I can't put it there. Or could it be a local static flag? Is there only one global module index per run of clang? Or is there one per module.map file? I can't put it in ModuleMap because of there being multiple instances of it. Could global module index objects be shared as well? Just to see if this recursion had some relationship to the module build error, I just put a temporary static flag in loadGlobalModuleIndex, but there was no change with respect to the module build error message. Sorry, I still don't know as much as I should about the modules system, but I figured you could save me some time figuring this out. Thanks. -John From: [email protected] [mailto:[email protected]] On Behalf Of Richard Smith Sent: Wednesday, March 26, 2014 5:13 PM To: Thompson, John Cc: [email protected]; [email protected] Subject: Re: [PATCH] preview patch for fixit for finding modules needing import/inclusion On Wed, Mar 26, 2014 at 4:19 PM, Thompson, John <[email protected]<mailto:[email protected]>> wrote: 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. I think a 'exact match, but missing import' correction is vastly more likely to be the one the user meant than a 'near match' correction, but I'm worried about the cost of doing this check. I suppose once we've committed to taking the performance cost, it's better to do this lookup earlier. Incidentally, I think you don't actually need to perform a lookup in your global module index code path -- just loading all the modules should be enough. Then the normal logic for finding hidden names from imported modules should find the name. > 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? One use case we want to support has (almost) all header files in modules, including user headers that change frequently, so (3) is likely to be expensive for that use case, if I understand what you're proposing. I'm torn between options 1 and 2. Mostly, I'm worried about the unpredictability of option 1. One other issue I just thought of: loading modules affects the semantics of the translation unit, even if those modules aren't made visible. Because a module is considered to be part of the program if it's loaded into a translation unit, we'll diagnose mismatched redeclarations. For instance, if this is in a module that's in one of the module maps on your include path: int foo; ... and you hit a typo correction and then see this: void foo(); ... you're likely to get a 'redefinition as a different kind of entity' diagnostic. This is not ideal. The big problem is that we sometimes call CorrectTypo when we're not sure that an error has occurred! You can find these cases easily enough: look for cases where ErrorRecovery == false in the call to diagnoseTypo. I suppose you could add an ErrorRecovery flag to CorrectTypo too, and only do your global module index import in the (common) error recovery case. -----Original Message----- From: [email protected]<mailto:[email protected]> [mailto:[email protected]<mailto:[email protected]>] On Behalf Of Richard Smith Sent: Wednesday, March 26, 2014 3:25 PM To: [email protected]<mailto:[email protected]>; [email protected]<mailto:[email protected]>; [email protected]<mailto:[email protected]> Cc: [email protected]<mailto:[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]<mailto:[email protected]> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected]<mailto:[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
