[PATCH] preview patch for fixit for finding modules needing import/inclusion

Richard Smith richard at metafoo.co.uk
Fri Apr 11 15:52:11 PDT 2014



================
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






More information about the cfe-commits mailing list