[PATCH] preview patch for fixit for finding modules needing import/inclusion
Thompson, John
John_Thompson at playstation.sony.com
Fri Apr 18 06:23:29 PDT 2014
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: 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
More information about the cfe-commits
mailing list