[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.

Many thanks! I didn't know about `split-file`, it's much nicer indeed!

Also incorporating feedback from @rsmith and removing the call to `makeMergedDefinitionVisible`. Keeping just `setPrimaryMergedDecl` is enough here.
Richard's reply from the email exchange:

  I don't think that makeMergedDefinitionVisible is quite right; that's intended to solve a different problem. Specifically, when we have a repeated class definition in a single parse:
  
  // in module X
  class A { void f(); };
  
  // in module Y
  class A { void f(); };
  
  Here, we first see and parse the class definition of A in module X. That then becomes "the" definition of A, for the purposes of this compilation. Then when we see another definition of the class, we choose to skip it because we already have a definition, and Clang wants there to only be one definition of each class. So we end up behaving as if we parsed this:
  
  // in module X
  class A { void f(); };
  
  // in module Y
  class A;
  
  ... but we still need to track that class A has a complete definition if only Y is imported, so that A::f can be used for example. And that's what makeMergedDefinitionVisible does: it says that A should be considered as having a complete definition when Y is imported, even though the definition actually lives in X.
  
  For concepts I think the situation is different: we're not going to skip the "body" of the concept and treat
  
  // module X
  template<typename T> concept bool C = true;
  
  // module Y
  template<typename T> concept bool C = true;
  
  as if it were
  
  
  // module X
  template<typename T> concept bool C = true;
  
  // module Y
  template<typename T> concept bool C; // forward declaration?
  
  ... because there's no such thing as a forward declaration of a concept. Instead, I think we should just be calling ASTContext::setPrimaryMergedDecl to say that the C in module Y is a redeclaration of the C in module X, so that a name lookup that finds both is not ambiguous. It shouldn't matter which one we actually pick, because they are the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128921