[NFC] follow up code cleanup after D123837

[ChuanqiXu]

Summary

I found there are chances to do simplify the codes further after I landed D123837. This one should be NFC only.

[ChuanqiXu]

clang/lib/Sema/SemaLookup.cpp

1573–1581

The definition of GlobalModuleFragment is The global module fragment of the current translation unit. So I feel it more natural to use this and I don't need to judge if it is GlobalModuleFragment anymore. The original looks more hack a little bit.

1581

And we don't need to check module private specially. So that we could filter more cases to avoid string comparison.

1784–1792

LLVM prefer shorter indentation. (Although I repeat ModulePrivate here, I feel the current one is better)

1787–1804

After D123837 is landed, the following two checks wouldn't be hit any more.

[ChuanqiXu]

@iains gentle ping~

[ChuanqiXu]

@iains ping!

[iains]

sorry for being slow, concentrating on module initialisers!

this LGTM (but we can still probably make the visibility code cleaner - perhaps after we get the main functionality installed).

[ChuanqiXu]

In D124149#3500423, @iains wrote:

sorry for being slow, concentrating on module initialisers!

this LGTM (but we can still probably make the visibility code cleaner - perhaps after we get the main functionality installed).

Yeah, of course we need further cleaner code.

clang/lib/Sema/SemaLookup.cpp

 
 /// Determine if we could use all the declarations in the module.
 bool Sema::isUsableModule(const Module *M) {
   assert(M && "We shouldn't check nullness for module here");
   // Return quickly if we cached the result.
   if (UsableModuleUnitsCache.count(M))
     return true;
 
-  // If M is the global module fragment of a module that we've not yet finished
-  // parsing, then M should be the global module fragment in current TU. So it
+  // If M is the global module fragment of the current translation unit. So it
   // should be usable.
   // [module.global.frag]p1:
   //   The global module fragment can be used to provide declarations that are
   //   attached to the global module and usable within the module unit.
-  if ((M->isGlobalModule() && !M->Parent) ||
-      // If M is the private module fragment, it is usable only if it is within
-      // the current module unit. And it must be the current parsing module unit
-      // if it is within the current module unit according to the grammar of the
-      // private module fragment.
-      // NOTE: This is covered by the following condition. The intention of the
-      // check is to avoid string comparison as much as possible.
-      (M->isPrivateModule() && M == getCurrentModule()) ||
+  if (M == GlobalModuleFragment ||
+      // If M is the module we're parsing, it should be usable. This covers the
+      // private module fragment. The private module fragment is usable only if
+      // it is within the current module unit. And it must be the current
+      // parsing module unit if it is within the current module unit according
+      // to the grammar of the private module fragment. NOTE: This is covered by
+      // the following condition. The intention of the check is to avoid string
+      // comparison as much as possible.
+      M == getCurrentModule() ||
       // The module unit which is in the same module with the current module
       // unit is usable.
       //
       // FIXME: Here we judge if they are in the same module by comparing the
       // string. Is there any better solution?
-      (M->getPrimaryModuleInterfaceName() ==
-       llvm::StringRef(getLangOpts().CurrentModule).split(':').first)) {
+      M->getPrimaryModuleInterfaceName() ==
+          llvm::StringRef(getLangOpts().CurrentModule).split(':').first) {
     UsableModuleUnitsCache.insert(M);
     return true;
   }
 
   return false;
 }
 
 bool Sema::hasVisibleMergedDefinition(NamedDecl *Def) {
if (DC && !IsEffectivelyFileContext(DC)) {
     return VisibleWithinParent;
   }
 
   return false;
 }
 
 bool Sema::isModuleVisible(const Module *M, bool ModulePrivate) {
   // The module might be ordinarily visible. For a module-private query, that
-  // means it is part of the current module. For any other query, that means it
-  // is in our visible module set.
-  if (ModulePrivate) {
-    if (isUsableModule(M))
-      return true;
-    else if (M->Kind == Module::ModuleKind::ModulePartitionImplementation &&
-             isModuleDirectlyImported(M))
-      // Unless a partition implementation is directly imported it is not
-      // counted as visible for lookup, although the contained decls might
-      // still be reachable.  It's a partition, so it must be part of the
-      // current module to be a valid import.
-      return true;
-    else if (getLangOpts().CPlusPlusModules && !ModuleScopes.empty() &&
-             ModuleScopes[0].Module->Kind ==
-                 Module::ModuleKind::ModulePartitionImplementation &&
-             ModuleScopes[0].Module->getPrimaryModuleInterfaceName() ==
-                 M->Name &&
-             isModuleDirectlyImported(M))
-      // We are building a module implementation partition and the TU imports
-      // the primary module interface unit.
-      return true;
-  } else {
-    if (VisibleModules.isVisible(M))
-      return true;
-  }
+  // means it is part of the current module.
+  if (ModulePrivate && isUsableModule(M))
+
+  // For a query which is not module-private, that means it is in our visible
+  // module set.
+  if (!ModulePrivate && VisibleModules.isVisible(M))
 
   // Otherwise, it might be visible by virtue of the query being within a
   // template instantiation or similar that is permitted to look inside M.
 
   // Find the extra places where we need to look.
   const auto &LookupModules = getLookupModules();
   if (LookupModules.empty())
     return false;