Richard, > If we really want to allow this as an extension (and I don't see why we > should), it should be an `ExtWarn`, and should be `DefaultError`.
Do mean you're pretty much against this fixit, I suppose because of the performance impact when it kicks in? -John -----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of Richard Smith Sent: Friday, April 11, 2014 3:52 PM To: [email protected]; [email protected]; [email protected] Cc: [email protected] Subject: Re: [PATCH] preview patch for fixit for finding modules needing import/inclusion ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6961-6963 @@ -6960,6 +6960,5 @@ "__module_private__">; -def err_module_private_declaration : Error< - "declaration of %0 must be imported from module '%1' before it is required">; -def err_module_private_definition : Error< - "definition of %0 must be imported from module '%1' before it is required">; +def warn_need_module_import : Warning< + "use of identifier '%0' requires import/inclusion of the module +'%1'">, + InGroup<NeedImport>; def err_module_import_in_extern_c : Error< ---------------- This shouldn't be a `Warning`. If we really want to allow this as an extension (and I don't see why we should), it should be an `ExtWarn`, and should be `DefaultError`. ================ Comment at: include/clang/Driver/Options.td:671-673 @@ -667,2 +670,5 @@ Flags<[DriverOption]>; +def fno_modules_search_all : Flag <["-"], "fno-modules-search-all">, +Group<f_Group>, + Flags<[DriverOption, CC1Option]>, + HelpText<"Inhibit search of non-imported modules to resolve +references">; def fno_ms_extensions : Flag<["-"], "fno-ms-extensions">, Group<f_Group>; ---------------- Only the non-default value of the flag should have `HelpText`. ================ Comment at: include/clang/Frontend/CompilerInstance.h:154 @@ -150,3 +153,3 @@ public: - CompilerInstance(); + CompilerInstance(bool BuildingModuleFlag = false); ~CompilerInstance(); ---------------- This constructor should be `explicit`. Also, just `BuildingModule`, not `BuildingModuleFlag`. ================ Comment at: lib/Frontend/CompilerInstance.cpp:857 @@ -853,3 +856,3 @@ // module. - CompilerInstance Instance; + CompilerInstance Instance(true); Instance.setInvocation(&*Invocation); ---------------- `Instance(/*BuildingModule=*/true);` would be more obvious. ================ Comment at: lib/Frontend/CompilerInstance.cpp:1427 @@ +1426,3 @@ + if (!HaveFullGlobalModuleIndex && GlobalIndex && !buildingModule()) { + ModuleMap &MMap = getPreprocessor().getHeaderSearchInfo().getModuleMap(); + bool recreateIndex = false; ---------------- Do we need to take any other steps to ensure we've actually loaded all the modulemap files that are within our include paths? ================ Comment at: lib/Frontend/CompilerInvocation.cpp:1347-1349 @@ -1346,2 +1346,5 @@ Opts.ModulesDeclUse = Args.hasArg(OPT_fmodules_decluse); + Opts.ModulesSearchAll = Opts.Modules && + (!Args.hasArg(OPT_fno_modules_search_all) || + Args.hasArg(OPT_fmodules_search_all)); Opts.CharIsSigned = Opts.OpenCL || !Args.hasArg(OPT_fno_signed_char); ---------------- I think this should default to off. It still makes diagnostics non-reproducible (because it depends on what other modules happen to be in the global index at the point when we perform typo correction), and may have a *very* significant runtime penalty if we hit a diagnostic. ================ Comment at: lib/Sema/SemaDecl.cpp:10072 @@ -10072,1 +10071,3 @@ + LookupOrdinaryName, S, 0, Validator, + 0, false, 0, true, false))) diagnoseTypo(Corrected, PDiag(diag::note_function_suggestion), ---------------- I really don't like this collection of unexplained mystery arguments (repeating the default arguments for all but the last). Maybe move the error recovery flag earlier, and change it to an `enum class` so its values can have more obvious names (and won't accidentally convert to `bool`)? ================ Comment at: lib/Sema/SemaLookup.cpp:4067 @@ +4066,3 @@ + if (ErrorRecovery && !Loader.buildingModule() + && getLangOpts().Modules && getLangOpts().ModulesSearchAll) { + // Load global module index, or retrieve a previously loaded one. ---------------- `&&` should go on the previous line. ================ Comment at: lib/Sema/SemaLookup.cpp:4075-4109 @@ +4074,37 @@ + + // Find the modules that reference the identifier. + // Note that this only finds top-level modules. + // We'll let diagnoseTypo find the actual declaration module. + if (GlobalIndex->lookupIdentifier(Typo->getName(), FoundModules)) { + TypoCorrection TC(TypoName.getName(), (NestedNameSpecifier *)0, 0); + TC.setCorrectionRange(SS, TypoName); + // Walk the found modules that reference the identifier. + for (GlobalModuleIndex::HitSet::iterator I = FoundModules.begin(), + E = FoundModules.end(); I != E; ++I) { + ModuleFile *TheModuleFile = *I; + // Find the module from the file name. + Module *TheModule = PP.getHeaderSearchInfo().lookupModuleFromFile( + TheModuleFile->FileName); + assert(TheModule && "Should be able to find the module."); + // Make the module visible so we can do a name lookup. + Loader.makeModuleVisible(TheModule, Module::AllVisible, + TypoName.getLoc(), false); + // Do a name lookup. + LookupResult ModRes(*this, TypoName, LookupKind); + LookupPotentialTypoResult(*this, ModRes, Typo, S, SS, MemberContext, + EnteringContext, OPT, false); + // If we have an exact match, save the decl in the coorection + // to let diagnoseTypo do a fixit message. + if (ModRes.getResultKind() == LookupResult::Found) + TC.setCorrectionDecl(ModRes.getAsSingle<NamedDecl>()); + // Hide the module again. diagnoseTypo will unhide the decl module. + Loader.makeModuleVisible(TheModule, Module::Hidden, + TypoName.getLoc(), false); + } + // If the name lookup found something, we set a flag to tell + // diagnoseTypo we have a case of possibly missing module import. + if (TC.isResolved()) { + TC.setRequiresImport(true); + return TC; + } + } ---------------- I think this is all unnecessary; just make sure we've loaded all the modules that contain the identifier, and let the normal recovery for an invisible name do the rest. ================ Comment at: lib/Sema/SemaLookup.cpp:4101-4102 @@ +4100,4 @@ + // Hide the module again. diagnoseTypo will unhide the decl module. + Loader.makeModuleVisible(TheModule, Module::Hidden, + TypoName.getLoc(), false); + } ---------------- You can't put the genie back in the bottle: once a module is unhidden, hiding it again doesn't work. ================ Comment at: lib/Sema/SemaLookup.cpp:4693-4694 @@ -4631,4 +4692,4 @@ - Diag(Correction.getCorrectionRange().getBegin(), - diag::err_module_private_declaration) - << Def << Owner->getFullModuleName(); + std::string fixit = "#import "; + fixit += Owner->Name; + SourceLocation TypoLoc = + Correction.getCorrectionRange().getBegin(); ---------------- This is not a correct fixit. ================ Comment at: lib/Sema/SemaLookup.cpp:4696-4697 @@ +4695,4 @@ + SourceLocation TypoLoc = Correction.getCorrectionRange().getBegin(); + // FIXME: Figure out how to find the right insertion point, + // For now we just use the type location. + Diag(TypoLoc, diag::warn_need_module_import) ---------------- This is also not OK. Please take out this fixit code for now; that's essentially orthogonal and we can deal with it as a separate change. ================ 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"); +} + ---------------- Please commit this as a separate change. http://reviews.llvm.org/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
