[PATCH] D130614: [C++20] [Modules] Merge same concept decls in global module fragment

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks for the thorough explanation and testing.
LGTM.



================
Comment at: clang/lib/Sema/SemaDecl.cpp:1738
+// Return true if the redefinition is not allowed. Return false otherwise.
+bool Sema::CheckRedefinitionInModule(const NamedDecl *New,
+                                     const NamedDecl *Old) const {
----------------
NIT: maybe rename to `IsRedefinitionInModule`?
I would expect `Check...` to produce a diagnostic itself. Having `Is...` in the name also removes any confusion about whether the `true` as a return value indicates an error or not.



================
Comment at: clang/lib/Sema/SemaDecl.cpp:1747
+
+  Module *NewM = New->getOwningModule();
+  Module *OldM = Old->getOwningModule();
----------------
NIT: maybe accept the modules directly?


================
Comment at: clang/test/Modules/merge-concepts.cppm:22
+// Testing with module map modules. It is unclear about the relation ship between Clang modules and
+// C++20 Named Modules. Will them co_exist? Or will them be mutual from each other?
+// The test here is for primarily coverity.
----------------
NIT on wording


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

https://reviews.llvm.org/D130614