[NFC] [C++20] [Modules] Simplify ActOnModuleImport by merging Path and Parition

[ChuanqiXu]

Summary

The parameter of Path and Partition of ActOnModuleImport could be merged and the implementation would be simpler.

[iains]

other than the two nits, LGTM

clang/include/clang/Sema/Sema.h

2996–2997

maybe:
"If the name is for a partition."

[iains]

clang/lib/Sema/SemaModule.cpp

415

it looks like there is an extra blank line before the closing brace..

[ChuanqiXu]

Address comments.

clang/lib/Sema/SemaModule.cpp

415

No, it isn't. It shows by the review page.

clang/include/clang/Sema/Sema.h

DeclGroupPtrTy ActOnPrivateModuleFragmentDecl(SourceLocation ModuleLoc,
                                                 SourceLocation PrivateLoc);
 
   /// The parser has processed a module import declaration.
   ///
   /// \param StartLoc The location of the first token in the declaration. This
   ///        could be the location of an '@', 'export', or 'import'.
   /// \param ExportLoc The location of the 'export' keyword, if any.
   /// \param ImportLoc The location of the 'import' keyword.
   /// \param Path The module toplevel name as an access path.
-  /// \param Partition The module partition name as an access path.
+  /// \param IsPartition If the name is for a partition.
   DeclResult ActOnModuleImport(SourceLocation StartLoc,
                                SourceLocation ExportLoc,
                                SourceLocation ImportLoc, ModuleIdPath Path,
-                               ModuleIdPath Partition = {});
+                               bool IsPartition = false);
   DeclResult ActOnModuleImport(SourceLocation StartLoc,
                                SourceLocation ExportLoc,
                                SourceLocation ImportLoc, Module *M,
                                ModuleIdPath Path = {});
 
   /// The parser has processed a module import translated from a
   /// #include or similar preprocessing directive.
   void ActOnModuleInclude(SourceLocation DirectiveLoc, Module *Mod);

clang/lib/Parse/Parser.cpp

Decl *Parser::ParseModuleImport(SourceLocation AtLoc,
   assert((AtLoc.isInvalid() ? Tok.isOneOf(tok::kw_import, tok::identifier)
                             : Tok.isObjCAtKeyword(tok::objc_import)) &&
          "Improper start to module import");
   bool IsObjCAtImport = Tok.isObjCAtKeyword(tok::objc_import);
   SourceLocation ImportLoc = ConsumeToken();
 
   // For C++20 modules, we can have "name" or ":Partition name" as valid input.
   SmallVector<std::pair<IdentifierInfo *, SourceLocation>, 2> Path;
-  SmallVector<std::pair<IdentifierInfo *, SourceLocation>, 2> Partition;
+  bool IsPartition = false;
   Module *HeaderUnit = nullptr;
   if (Tok.is(tok::header_name)) {
     // This is a header import that the preprocessor decided we should skip
     // because it was malformed in some way. Parse and ignore it; it's already
     // been diagnosed.
     ConsumeToken();
   } else if (Tok.is(tok::annot_header_unit)) {
     // This is a header import that the preprocessor mapped to a module import.
     HeaderUnit = reinterpret_cast<Module *>(Tok.getAnnotationValue());
     ConsumeAnnotationToken();
   } else if (Tok.is(tok::colon)) {
     SourceLocation ColonLoc = ConsumeToken();
     if (!getLangOpts().CPlusPlusModules)
       Diag(ColonLoc, diag::err_unsupported_module_partition)
-          << SourceRange(ColonLoc, Partition.back().second);
+          << SourceRange(ColonLoc, Path.back().second);
     // Recover by leaving partition empty.
-    else if (ParseModuleName(ColonLoc, Partition, /*IsImport*/ true))
+    else if (ParseModuleName(ColonLoc, Path, /*IsImport*/ true))
       return nullptr;
+    else
+      IsPartition = true;
   } else {
     if (ParseModuleName(ImportLoc, Path, /*IsImport*/ true))
       return nullptr;
   }
 
   ParsedAttributesWithRange Attrs(AttrFactory);
   MaybeParseCXX11Attributes(Attrs);
   // We don't support any module import attributes yet.
   ProhibitCXX11Attributes(Attrs, diag::err_attribute_not_import_attr);
 
   if (PP.hadModuleLoaderFatalFailure()) {
     // With a fatal failure in the module loader, we abort parsing.
     cutOffParsing();
     return nullptr;
   }
 
   // Diagnose mis-imports.
   bool SeenError = true;
-  bool HasPart = !Partition.empty();
   switch (ImportState) {
   case Sema::ModuleImportState::ImportAllowed:
     SeenError = false;
     break;
   case Sema::ModuleImportState::FirstDecl:
   case Sema::ModuleImportState::NotACXX20Module:
     // We can only import a partition within a module purview.
-    if (HasPart)
+    if (IsPartition)
       Diag(ImportLoc, diag::err_partition_import_outside_module);
     else
       SeenError = false;
     break;
   case Sema::ModuleImportState::GlobalFragment:
     // We can only have pre-processor directives in the global module
     // fragment.  We can, however have a header unit import here.
     if (!HeaderUnit)
-      Diag(ImportLoc, diag::err_import_in_wrong_fragment) << HasPart << 0;
+      Diag(ImportLoc, diag::err_import_in_wrong_fragment) << IsPartition << 0;
     else
       SeenError = false;
     break;
   case Sema::ModuleImportState::ImportFinished:
     if (getLangOpts().CPlusPlusModules)
       Diag(ImportLoc, diag::err_import_not_allowed_here);
     else
       SeenError = false;
     break;
   case Sema::ModuleImportState::PrivateFragment:
-    Diag(ImportLoc, diag::err_import_in_wrong_fragment) << HasPart << 1;
+    Diag(ImportLoc, diag::err_import_in_wrong_fragment) << IsPartition << 1;
     break;
   }
   if (SeenError) {
     ExpectAndConsumeSemi(diag::err_module_expected_semi);
     return nullptr;
   }
 
   DeclResult Import;
   if (HeaderUnit)
     Import =
         Actions.ActOnModuleImport(StartLoc, ExportLoc, ImportLoc, HeaderUnit);
-  else if (!Path.empty() || !Partition.empty())
+  else if (!Path.empty())
     Import = Actions.ActOnModuleImport(StartLoc, ExportLoc, ImportLoc, Path,
-                                       Partition);
+                                       IsPartition);
   ExpectAndConsumeSemi(diag::err_module_expected_semi);
   if (Import.isInvalid())
     return nullptr;
 
   // Using '@import' in framework headers requires modules to be enabled so that
   // the header is parseable. Emit a warning to make the user aware.
   if (IsObjCAtImport && AtLoc.isValid()) {
     auto &SrcMgr = PP.getSourceManager();

clang/lib/Sema/SemaModule.cpp

Sema::ActOnPrivateModuleFragmentDecl(SourceLocation ModuleLoc,
 
   // FIXME: Consider creating an explicit representation of this declaration.
   return nullptr;
 }
 
 DeclResult Sema::ActOnModuleImport(SourceLocation StartLoc,
                                    SourceLocation ExportLoc,
                                    SourceLocation ImportLoc, ModuleIdPath Path,
-                                   ModuleIdPath Partition) {
+                                   bool IsPartition) {
 
-  bool IsPartition = !Partition.empty();
   bool Cxx20Mode = getLangOpts().CPlusPlusModules || getLangOpts().ModulesTS;
   assert((!IsPartition || Cxx20Mode) && "partition seen in non-C++20 code?");
-  assert((!IsPartition || Path.empty()) &&
-         "trying to import a partition with its named module specified?");
 
   // For a C++20 module name, flatten into a single identifier with the source
   // location of the first component.
   std::pair<IdentifierInfo *, SourceLocation> ModuleNameLoc;
 
   std::string ModuleName;
   if (IsPartition) {
     // We already checked that we are in a module purview in the parser.
     assert(!ModuleScopes.empty() && "in a module purview, but no module?");
     Module *NamedMod = ModuleScopes.back().Module;
     // If we are importing into a partition, find the owning named module,
     // otherwise, the name of the importing named module.
     ModuleName = NamedMod->getPrimaryModuleInterfaceName().str();
     ModuleName += ":";
-    ModuleName += stringFromPath(Partition);
-    ModuleNameLoc = {PP.getIdentifierInfo(ModuleName), Partition[0].second};
-    Partition = ModuleIdPath(ModuleNameLoc);
+    ModuleName += stringFromPath(Path);
+    ModuleNameLoc = {PP.getIdentifierInfo(ModuleName), Path[0].second};
+    Path = ModuleIdPath(ModuleNameLoc);
   } else if (Cxx20Mode) {
     ModuleName = stringFromPath(Path);
     ModuleNameLoc = {PP.getIdentifierInfo(ModuleName), Path[0].second};
     Path = ModuleIdPath(ModuleNameLoc);
   }
 
   // Diagnose self-import before attempting a load.
   // [module.import]/9
   // A module implementation unit of a module M that is not a module partition
   // shall not contain a module-import-declaration nominating M.
   // (for an implementation, the module interface is imported implicitly,
   //  but that's handled in the module decl code).
 
   if (getLangOpts().CPlusPlusModules && isCurrentModulePurview() &&
       getCurrentModule()->Name == ModuleName) {
     Diag(ImportLoc, diag::err_module_self_import_cxx20)
         << ModuleName << !ModuleScopes.back().ModuleInterface;
     return true;
   }
 
   Module *Mod = getModuleLoader().loadModule(
-      ImportLoc, IsPartition ? Partition : Path, Module::AllVisible,
-      /*IsInclusionDirective=*/false);
+      ImportLoc, Path, Module::AllVisible, /*IsInclusionDirective=*/false);
   if (!Mod)
     return true;
 
-  return ActOnModuleImport(StartLoc, ExportLoc, ImportLoc, Mod,
-                           IsPartition ? Partition : Path);
+  return ActOnModuleImport(StartLoc, ExportLoc, ImportLoc, Mod, Path);
 }
 
 /// Determine whether \p D is lexically within an export-declaration.
 static const ExportDecl *getEnclosingExportDecl(const Decl *D) {
   for (auto *DC = D->getLexicalDeclContext(); DC; DC = DC->getLexicalParent())
     if (auto *ED = dyn_cast<ExportDecl>(DC))
       return ED;
   return nullptr;
 }