On Fri, Apr 18, 2014 at 6:23 AM, Thompson, John <[email protected]> wrote: > 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?
I'm not sure how that follows from Richard's comment. Errors can have fixits, and indeed the cost of computing a fixit on an error is less important (ie: the cost can be higher) since we're going to reject compilation anyway. (with a warning, someone might've turned the warning off - or be ignoring it otherwise, and we don't want to slow down their build much if we can help it) > > -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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
