[PATCH] preview patch for fixit for finding modules needing import/inclusion
David Blaikie
dblaikie at gmail.com
Fri Apr 18 08:23:44 PDT 2014
On Fri, Apr 18, 2014 at 6:23 AM, Thompson, John
<John_Thompson at playstation.sony.com> 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: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Richard Smith
> Sent: Friday, April 11, 2014 3:52 PM
> To: dgregor at apple.com; richard at metafoo.co.uk; john.thompson.jtsoftware at gmail.com
> Cc: cfe-commits at cs.uiuc.edu
> 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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list