[PATCH] D128921: [Sema] Merge C++20 concept definitions from different modules in same TU

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I haven't seen your reply before posting my initial comments. Many thanks for a very quick turnaroud on this!
Will wait for module folks to approve.



================
Comment at: clang/lib/Sema/SemaTemplate.cpp:8674
     auto *Old = Previous.getRepresentativeDecl();
     Diag(NameLoc, isa<ConceptDecl>(Old) ? diag::err_redefinition :
          diag::err_redefinition_different_kind) << NewDecl->getDeclName();
----------------
ilya-biryukov wrote:
> Not sure if reporting the redefinition if `isSameEntity` returns false is the right approach here.
> This leads to errors on definitions of something like:
> ```
> // foo.h
> template <class T> concept A = false;
> 
> // bar.h
> template <class T, class U> concept A = true;
> 
> #include "foo.h"
> #include "bar.h" // <-- redefinition error shown here
> ```
> 
> The alternative is the "ambiguous reference" error on the use of the concept. I will check what happens for variables and typedefs in that case and follow the same pattern.
Clang tends to do "ambiguous reference" instead of the behavior we had in this patch.
I have updated it to be consistent with what typedefs and other symbols produce.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128921