[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! Mostly questions to better understand the spec and clang. Please excuse me if they sound too basic.
I have read the modules section of the standard, but I couldn't find where it specifies whether these redefinition errors should be present or not. Any guidance of where to look at?

Could we add a few examples where legitimate redefinition errors do happen?
(private module fragment?



================
Comment at: clang/lib/Sema/SemaTemplate.cpp:8731
 
-  auto *OldConcept = dyn_cast<ConceptDecl>(Previous.getRepresentativeDecl());
+  NamedDecl *Old = Previous.getRepresentativeDecl();
+  if (auto *USD = dyn_cast<UsingShadowDecl>(Old))
----------------
This follows what `getFoundDecl()` does, but still allows to show errors when lookup results contain multiple entities.

A question too: is true that concept is always reachable if it has a reachable using decl?
I wonder whether `hasReachableDefinition` should be called on `UsingShadowDecl` instead?


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:8755
+      // old one, we should merge them instead of complaining.
+      !(OldConcept->getOwningModule() &&
+        OldConcept->getOwningModule()->isGlobalModule())) {
----------------
Would we allow redefinitions inside the same global module fragment?
```
module;

template<class T> concept ok = true;
template<class T> concept ok = true; // should be an error.
```

could we add a test for that?
or do we only mark declarations coming from actual pcm files as having as owning module fragment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130614