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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a reviewer: ChuanqiXu.
ChuanqiXu added a comment.

Thanks for filing that issue. I wanted to add that myself before. It should be better to change `isVisible` and `hasVisibleDefinition` to `isReachable` and `hasReachableDefinition`. The definition of reachability comes from C++20 Modules and it shouldn't affect Clang Modules.

And would you like to add an example for C++20 Modules? It should be something like:

  // RUN: rm -rf %t
  // RUN: mkdir %t
  // RUN: split-file %s %t
  //
  // RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/same_as.cppm -o %t/same_as.pcm
  // RUN: %clang_cc1 -std=c++20 -emit-module-interface -fprebuilt-module-path=%t %t/concepts.cppm -o %t/concepts.pcm
  // RUN: %clang_cc1 -std=c++20 -emit-module-interface -fprebuilt-module-path=%t %t/format.cppm -o %t/format.pcm
  // RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/conflicting.cppm -o %t/conflicting.pcm
  // RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/Use.cppm -fsyntax-only -verify
  
  //--- same_as.cppm
  export module same_as;
  export template <class T, class U>
  concept same_as = __is_same(T, U);
  
  //--- concepts.cppm
  export module concepts;
  export import same_as;
  
  export template <class T>
  concept ConflictingConcept = true;
  
  //--- format.cppm
  
  export module format;
  export import concepts;
  export import same_as;
  
  export template <class T> void foo()
    requires same_as<T, int>
  {}
  
  //--- conflicting.cppm
  export module conflicting;
  export template <class T, class U = int>
  concept ConflictingConcept = true;
  
  //--- Use.cppm
  import format;
  import conflicting;
  
  template <class T> void foo()
    requires same_as<T, T>
  {}
  ConflictingConcept auto x = 10; // expected-error {{reference to 'ConflictingConcept' is ambiguous}}
                                  // expected-note@* 2 {{candidate}}

Also I feel it would be better to rewrite the current tests into the above style by `split-file`.



================
Comment at: clang/lib/Sema/SemaTemplate.cpp:8679
+      Previous.isSingleResult() ? Previous.getAsSingle<ConceptDecl>() : nullptr;
+  if (PrevDef && !hasVisibleDefinition(PrevDef) &&
+      Context.isSameEntity(NewDecl, PrevDef)) {
----------------



================
Comment at: clang/lib/Sema/SemaTemplate.cpp:8689
+  while (F.hasNext()) {
+    if (!isVisible(F.next())) {
+      F.erase();
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128921