[PATCH] D124149: [NFC] follow up code cleanup after D123837

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/lib/Sema/SemaLookup.cpp:1573
   //   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
----------------
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.


================
Comment at: clang/lib/Sema/SemaLookup.cpp:1581
+      // comparison as much as possible.
+      M == getCurrentModule() ||
       // The module unit which is in the same module with the current module
----------------
And we don't need to check module private specially. So that we could filter more cases to avoid string comparison.


================
Comment at: clang/lib/Sema/SemaLookup.cpp:1784-1790
+  // means it is part of the current module.
+  if (ModulePrivate && isUsableModule(M))
+    return true;
+
+  // For a query which is not module-private, that means it is in our visible
+  // module set.
+  if (!ModulePrivate && VisibleModules.isVisible(M))
----------------
LLVM prefer shorter indentation. (Although I repeat `ModulePrivate` here, I feel the current one is better)


================
Comment at: clang/lib/Sema/SemaLookup.cpp:1787-1804
-    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
----------------
After D123837 is landed, the following two checks wouldn't be hit any more.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124149/new/

https://reviews.llvm.org/D124149