This is an archive of the discontinued LLVM Phabricator instance.

[C++20] [Module] Support reachable definition initially/partially
ClosedPublic

Authored by ChuanqiXu on Nov 10 2021, 12:11 AM.

Details

Summary

This fixes: https://bugs.llvm.org/show_bug.cgi?id=52281 and https://godbolt.org/z/81f3ocjfW.

This patch introduces a new kind of ModuleOwnershipKind as ReachableWhenImported. This intended the status for reachable described at: https://eel.is/c++draft/module.reach#3.

Note that this patch is not intended to support all semantics about reachable semantics. For example, this patch didn't implement discarded declarations in GMF. (https://eel.is/c++draft/module.global.frag#3).

According to my experience, this patch is important to use C++20 module actually. After this patch, people should be able to start to use module in practice. (There may be other bugs. I know some too).

Test Plan: check-all and https://godbolt.org/z/81f3ocjfW.

Diff Detail

Event Timeline

ChuanqiXu created this revision.Nov 10 2021, 12:11 AM
ChuanqiXu requested review of this revision.Nov 10 2021, 12:11 AM
ChuanqiXu updated this revision to Diff 393028.Dec 8 2021, 10:11 PM

Update comments.

ChuanqiXu planned changes to this revision.Dec 9 2021, 3:48 AM

I think it would be better to resent this when it is more complete.

ChuanqiXu updated this revision to Diff 410270.Feb 21 2022, 4:18 AM
ChuanqiXu edited the summary of this revision. (Show Details)
ChuanqiXu added a reviewer: iains.

Implement reachable definition initially/partially.

Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 6:20 PM
ChuanqiXu updated this revision to Diff 412657.Mar 3 2022, 3:16 AM

Rebase and update tests.

urnathan resigned from this revision.Mar 9 2022, 10:54 AM
ChuanqiXu added a reviewer: Restricted Project.Mar 20 2022, 11:15 PM

Thanks for working on this!

clang/include/clang/AST/DeclBase.h
228
230
clang/lib/Sema/SemaLookup.cpp
1615

I think this should be AllowReachable not RequireReachable, since what it means is "allow default arguments that are reachable but not visible".

But more generally I'd like to see this bool replaced by an enum, eg:

enum class AcceptableKind { Visible, Reachable };

... and rename functions like this that take the enum to hasAcceptableDefaultArgument. It'd be nice to still provide hasVisibleFoo and hasReachableFoo as simple wrappers around the function that has the enum parameter to make the call sites more readable, but we should try to not duplicate the definitions of these functions.

1622–1625

There's no need to check both visibility and reachability here.

Also, you shouldn't be asking whether D has any reachable redeclaration; you should be asking whether D itself is reachable.

What I'd really like to see here is:

if (S.isAcceptable(D, Kind))

with a Sema::isAcceptable(const Decl *D, AcceptableKind Kind)

1648–1650

Please remove this check: we should define what "reachable" means for module map modules, even if we define it as being the same as "visible", rather than adding special cases that try to distinguish the two modes. We support combining module map modules and C++ modules in the same compilation, so we need a semantic model that supports both at once.

1672–1675

As previously, it's redundant to check both. (And if there were somehow a declaration that was visible but not reachable, this would be wrong -- but that should never happen.)

1709–1755

I don't like duplicating this code for the reachable / visible checks. Instead, please add a parameter for the kind (visible / reachable).

1859–1860

This is wrong; whether the declaration came from an AST file is irrelevant. Keep in mind that declarations in the same source file may come from an AST file if we're using a PCH, and declarations in a different source file may *not* come from an AST file if parse multiple source files into the same ASTContext. What we need to check here is whether M is the global module fragment of the current source file.

2076–2080

I don't think this is quite right. Given:

export module M {
export enum E1 { e1 };
enum E2 { e2 };
export using E2U = E2;
enum E3 { e3 };
export E3 f();

... the intent is:

import M;
int main() {
  auto a = E1::e1; // OK, namespace-scope name E1 is visible and e1 is reachable
  auto b = e1; // OK, namespace-scope name e1 is visible
  auto c = E2::e2; // error, namespace-scope name E2 is not visible
  auto d = e2; // error, namespace-scope name e2 is not visible
  auto e = E2U::e2; // OK, namespace-scope name E2U is visible and E2::e2 is reachable
  auto f = E3::e3; // error, namespace-scope name E3 is not visible
  auto g = e3; // error, namespace-scope name e3 is not visible
  auto h = decltype(f())::e3; // OK, namespace-scope name f is visible and E3::e3 is reachable
}

Instead of doing the check in this function, I think we need to consider the scope in which we're doing a lookup: if that scope is a class or enumeration scope, and the class or enumeration has a reachable definition, then we don't do any visibility checks for its members.

3859

We should look for a reachable definition here, not a reachable declaration.

rsmith added inline comments.Apr 13 2022, 2:41 PM
clang/lib/Sema/SemaLookup.cpp
2090

We should look for a reachable definition, not a reachable declaration.

clang/lib/Sema/SemaModule.cpp
340

This should not depend on whether the C++ modules language feature is enabled, it should depend on whether the current module is a module map module versus a C++ module.

Module map module semantics should be the same regardless of whether we're in C++20 mode.

clang/lib/Sema/SemaTemplate.cpp
11014

Use the enum here.

clang/lib/Sema/SemaType.cpp
8660

This can be a slow check; it'd be better to avoid doing it if we're in the overwhelmingly common case that the declaration is obviously reachable (because it's not module-private, and we're not trying to implement strict reachability semantics).

8674–8676

This isn't quite right: the same entity could have a declaration in a module map module and another declaration in a C++ header module. We need to check whether the entity has *any* declaration in a C++ module, not only whether D is in one.

8678–8681

A declaration not being from an AST file should not be assumed to imply that it appears in the same TU. A Clang-as-a-library user might parse multiple source files into a single ASTContext, and ASTImporter can be used to combine multiple source files into a single ASTContext.

8710–8713

We should have a flag here to control this behavior. The default should probably be that we enforce reachability as strictly as we can, for portability and to help users enforce an "import what you use" hygiene rule, but in any case, we should give users the choice.

clang/lib/Serialization/ASTReaderDecl.cpp
617–619

This doesn't explain what's special about the number 4. It would be better to say "Unknown ModuleOwnership kind". It might be better still to use a switch here, so that we get a compile-time error rather than a runtime error if a new kind is added and this code isn't updated.

ChuanqiXu updated this revision to Diff 423044.Apr 15 2022, 1:38 AM
ChuanqiXu marked 18 inline comments as done.

Address @rsmith's comments.

@rsmith Thanks for your valuable input!

clang/lib/Sema/SemaLookup.cpp
2076–2080

Oh, your example makes sense. The current implementation would accept d and g unexpectedly. I spent some time to look into this one. And I find it is not so easy to fix. I left a FIXME below and I would file an issue if this patch landed. Do you think this is OK?

BTW, I feel like the wording of spec need to be adjusted too. From the wording, I feel like d and g should be accepted.

3859

Yeah, from the wording it should be reachable definition. But it would cause many failures if I write hasReachableDefinition/hasVisibleDefinition here. So it looks like a legacy defect. I chose to follow the original style here.

Rebase with main.

ChuanqiXu updated this revision to Diff 428263.May 9 2022, 7:18 PM

Code Cleanups

iains added a comment.Jun 6 2022, 11:04 AM

I'm going to try rebasing my GMF elision code on top of this.

the test-suite changes could be made easier to read by using the file-split stuff we've been using for recent changes (I guess this is an older patch)?

clang/include/clang/AST/DeclBase.h
234–237

that is not going to be sufficient to exclude them - see D126694, we introduce a new category "ModuleUnreachable".

636–637

see D126694.

clang/lib/Sema/SemaLookup.cpp
1947

I think isModuleInterfaceUnit needs to include implementation partition units, since they also have a BMI (is isInterfaceOrPartition() the right thing to use here?

1958

could we separate this part of the change into a separate patch (along with the command line switch etc.) ?
it is not needed for standard compliance, right?

clang/lib/Serialization/ASTWriterDecl.cpp
1931

ModuleOwnershipKind? (and several cases below)

ChuanqiXu updated this revision to Diff 434707.Jun 7 2022, 12:26 AM
ChuanqiXu marked 3 inline comments as done.

Address comments.

ChuanqiXu marked 2 inline comments as done.Jun 7 2022, 12:39 AM

the test-suite changes could be made easier to read by using the file-split stuff we've been using for recent changes (I guess this is an older patch)?

Done, this is pretty old. BTW, I've made an implementation internally and it is able to compile https://github.com/alibaba/async_simple/tree/CXX20Modules and this patch is main part of that. I want to say this one is tested more or less.


(This section might not be related to this revision)

Boris tried to compile it by GCC: https://github.com/boris-kolpackov/async_simple/tree/CXX20Modules but GCC fall in ICE

You could use this one as a (not complete) test suite.

clang/include/clang/AST/DeclBase.h
234–237

Yeah, I am not going to edit this to avoid unnecessary rebasing work.

clang/lib/Sema/SemaLookup.cpp
1947

I think we couldn't use isInterfaceOrPartition() here. The call for isModuleInterfaceUnit() here is sufficient. For example, we could find the example at [[ https://eel.is/c++draft/module.reach#example-1 | [module.reach]example 1 ]], here in Translation unit #5:, the definition of struct B is not reachable since the definition lives in an implementation unit. (We could make it valid by making all additional TU as reachable)

Also the module interface unit shouldn't include implementation partition unit according to the wording: [[ https://eel.is/c++draft/module.unit#2 | [module.unit]p2 ]]. I agree that it is confusing since implementation partition unit is importable. But this is not our fault.

ChuanqiXu marked an inline comment as done.Jun 7 2022, 12:57 AM
iains added a comment.Jun 7 2022, 1:04 AM

thanks for amending the test cases they are now much easier to read and maintain.

This looks like a useful step forward, hopefully we can resolve any outstanding issue soon and get it landed.

Did you try wider testing (e.g. with lldb and other parts of the toolchain?)

clang/lib/Sema/SemaLookup.cpp
1947

OK, perhaps I am missing something - just to clarify,...

In this (which I believe is legal like [module.unit] ex 1 TU 4.

module;
....
module Main;

import :ImplUnit; // this has a BMI which could contain reachable definitions, right?

...

Did you try wider testing (e.g. with lldb and other parts of the toolchain?)

Yeah, I tried some other testing including using modules slightly in SPEC2017 and some other C++ performance benchmarks to make sure the introduction of C++ modules wouldn't cause performance regression. But testing is never enough : )

ChuanqiXu added inline comments.Jun 7 2022, 1:16 AM
clang/lib/Sema/SemaLookup.cpp
1947

Yeah, I believe this is legal according to [module.reach]p1:

A translation unit U is necessarily reachable from a point P if U is a module interface unit on which the translation unit containing P has an interface dependency, or the translation unit containing P imports U, in either case prior to P.

Since module Main imports :ImplUnit directly, the :ImplUnit TU is necessarily reachable.

iains added inline comments.Jun 7 2022, 1:58 AM
clang/lib/Sema/SemaLookup.cpp
1947

(sorry for multiple iterations - I am trying to see if I missed some point ... )

... it seems to me that in valid code :ImplUnit can have Kind =
ModulePartitionInterface
or
ModulePartitionImplementation

the second is the special case of an implementation that provides a BMI also.

ChuanqiXu added inline comments.Jun 7 2022, 2:28 AM
clang/lib/Sema/SemaLookup.cpp
1947

Yes, this confused me for a relative long time. It is really confusing that module partition implementation unit is importable but not module interface unit. The standard often emphasize module interface unit. From my implementation and using experience, the implementor and user could treat module partition implementation unit as an implementation partition unit if we treat all additional implementation TU as reachable. (This is what we do internally)

@iains Given @rsmith is too busy, would you like to review this one?

iains added a comment.Jun 21 2022, 3:19 AM

@ChuanqiXu - have you tried the test example from 10.6 ex1 - I did the impl as below, but the behaviour does not seem to be quite correct?
any thoughts?

As for review, I can try to pick up the "nits" but not sure that I know the instantiation sub=system too well, so it would be better if @rsmith could cast an eye over those parts.

let's discuss the 10.6 example first (I'd guess 10.7 cases will need to be reviewed too)

// RUN: rm -rf %t
// RUN: split-file %s %t
// RUN: cd %t

// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-header-unit-header std-10-6-ex1-decl.h \
// RUN: -o decl.pcm

// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-header-unit-header std-10-6-ex1-defn.h \
// RUN: -o defn.pcm

// RUN: %clang_cc1 -std=c++20 -emit-module-interface std-10-6-ex1-stuff.cpp \
// RUN: -o stuff.pcm

// RUN: %clang_cc1 -std=c++20 -emit-module-interface std-10-6-ex1-M1.cpp \
// RUN: -fmodule-file=stuff.pcm -o M1.pcm  -fmodule-file=defn.pcm

// RUN: %clang_cc1 -std=c++20 -emit-module-interface std-10-6-ex1-M2.cpp \
// RUN: -fmodule-file=stuff.pcm -o M2.pcm  -fmodule-file=decl.pcm

// RUN: %clang_cc1 -std=c++20 std-10-6-ex1-use.cpp \
// RUN: -fmodule-file=M1.pcm -fmodule-file=M2.pcm  -fsyntax-only

//--- std-10-6-ex1-decl.h
struct X;

//--- std-10-6-ex1-defn.h
struct X {};

//--- std-10-6-ex1-stuff.cpp
export module stuff;
export template<typename T, typename U> void foo(T, U u) { auto v = u; }
export template<typename T, typename U> void bar(T, U u) { auto v = *u; }

//--- std-10-6-ex1-M1.cpp
export module M1;
import "std-10-6-ex1-defn.h"; // provides struct X {};
import stuff;

export template<typename T> void f(T t) {
  X x;
  foo(t, x); 
}

//--- std-10-6-ex1-M2.cpp
export module M2;
import "std-10-6-ex1-decl.h"; // provides struct X; (not a definition)

import stuff;
export template<typename T> void g(T t) {
  X *x;
  bar(t, x); 
}

//--- std-10-6-ex1-use.cpp
import M1;
import M2;

void test() {
  f(0);
  g(0);
}

(I think we might also have some issues with poor diagnostics for items in PMF (saying that an import is required before use, but of course that import is impossible - e.g. 10.5)

iains added a comment.Jun 21 2022, 3:48 AM

as suspected, we also have a missing diagnostic in 10.7 ex1

// RUN: rm -rf %t
// RUN: split-file %s %t
// RUN: cd %t

// RUN: %clang_cc1 -std=c++20 -emit-module-interface std-10-7-ex1-M-A.cpp \
// RUN: -o M-A.pcm

// RUN: %clang_cc1 -std=c++20 -emit-module-interface std-10-7-ex1-M-B.cpp \
// RUN: -o M-B.pcm

// RUN: %clang_cc1 -std=c++20 -emit-module-interface std-10-7-ex1-M-C.cpp \
// RUN: -fmodule-file=M-A.pcm -verify -fsyntax-only

// RUN: %clang_cc1 -std=c++20 -emit-module-interface std-10-7-ex1-M.cpp \
// RUN: -o M.pcm -fmodule-file=M-A.pcm -fmodule-file=M-B.pcm

// RUN: %clang_cc1 -std=c++20 -emit-module-interface std-10-7-ex1-X.cpp \
// RUN: -o X.pcm

// RUN: %clang_cc1 -std=c++20 std-10-7-ex1-use.cpp -fsyntax-only \
// RUN: -fmodule-file=M.pcm -fmodule-file=X.pcm -verify

//--- std-10-7-ex1-M-A.cpp
export module M:A;
export struct B;

//--- std-10-7-ex1-M-B.cpp
module M:B;
struct B {
  operator int();
};

//--- std-10-7-ex1-M-C.cpp
module M:C;
import :A;
B b1;   // expected-error {{variable has incomplete type 'B'}} 
        // expected-note@std-10-7-ex1-M-A.cpp:2 {{forward declaration of 'B'}}
//--- std-10-7-ex1-M.cpp
export module M;
export import :A;
import :B;
B b2;
export void f(B b = B());

//--- std-10-7-ex1-X.cpp
export module X;

//--- std-10-7-ex1-use.cpp
module X;
import M;
B b3;     // expected-error 1+{{definition of 'B' must be imported from module 'M:B' before it is required}} 
          // expected-note@std-10-7-ex1-M-B.cpp:2 1+{{definition here is not reachable}}
void g() { f(); }  // expected-error {{definition of 'B' must be imported from module 'M:B' before it is required}}
  // not sure of the actual diagnostic here ^ , it is not emitted.
ChuanqiXu added a comment.EditedJun 22 2022, 12:22 AM

As for review, I can try to pick up the "nits" but not sure that I know the instantiation sub=system too well, so it would be better if @rsmith could cast an eye over those parts.

Yeah, @rsmith should be the best one to review this. But as you can find, this revision is opened for months... and the revision blocks many uses of C++20 modules. I really hope someone could push this forward... This was tested before internally.

let's discuss the 10.6 example first (I'd guess 10.7 cases will need to be reviewed too)

// RUN: rm -rf %t
// RUN: split-file %s %t
// RUN: cd %t

// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-header-unit-header std-10-6-ex1-decl.h \
// RUN: -o decl.pcm

// RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-header-unit-header std-10-6-ex1-defn.h \
// RUN: -o defn.pcm

// RUN: %clang_cc1 -std=c++20 -emit-module-interface std-10-6-ex1-stuff.cpp \
// RUN: -o stuff.pcm

// RUN: %clang_cc1 -std=c++20 -emit-module-interface std-10-6-ex1-M1.cpp \
// RUN: -fmodule-file=stuff.pcm -o M1.pcm  -fmodule-file=defn.pcm

// RUN: %clang_cc1 -std=c++20 -emit-module-interface std-10-6-ex1-M2.cpp \
// RUN: -fmodule-file=stuff.pcm -o M2.pcm  -fmodule-file=decl.pcm

// RUN: %clang_cc1 -std=c++20 std-10-6-ex1-use.cpp \
// RUN: -fmodule-file=M1.pcm -fmodule-file=M2.pcm  -fsyntax-only

//--- std-10-6-ex1-decl.h
struct X;

//--- std-10-6-ex1-defn.h
struct X {};

//--- std-10-6-ex1-stuff.cpp
export module stuff;
export template<typename T, typename U> void foo(T, U u) { auto v = u; }
export template<typename T, typename U> void bar(T, U u) { auto v = *u; }

//--- std-10-6-ex1-M1.cpp
export module M1;
import "std-10-6-ex1-defn.h"; // provides struct X {};
import stuff;

export template<typename T> void f(T t) {
  X x;
  foo(t, x); 
}

//--- std-10-6-ex1-M2.cpp
export module M2;
import "std-10-6-ex1-decl.h"; // provides struct X; (not a definition)

import stuff;
export template<typename T> void g(T t) {
  X *x;
  bar(t, x); 
}

//--- std-10-6-ex1-use.cpp
import M1;
import M2;

void test() {
  f(0);
  g(0);
}

Thanks for reporting this. I've add the tests. And https://eel.is/c++draft/module.context#7.4 says it is unspecified whether or not g(0) is valid. So I think the current behavior is valid here.

as suspected, we also have a missing diagnostic in 10.7 ex1

Yes, this was added before at clang/test/CXX/module/module.reach/ex1.cpp. And I add a FIXME there:

// FIXME: We should emit an error for unreachable definition of B.
void g() { f(); }

I'll file an issue for it once this landed. BTW, the compiler would behave properly if:

//--- M.cppm
export module M;
export import :A;
import :B;
export B foo();
export void f(B b = B());
//--- Use.cpp
export module X;
import M;
void g() { f(foo()); } // complains successfully

This looks even more straight forward.

(I think we might also have some issues with poor diagnostics for items in PMF (saying that an import is required before use, but of course that import is impossible - e.g. 10.5)

Yes, there are many problems. And as the title said, this is an initial implementation and these problems are not regression bugs so I feel like it should be good to fix them in later versions.

Update comments for the test of [module.context]p7.

iains added a comment.Jun 22 2022, 1:13 AM

I think it is helpful to collect the standard examples into one place (i.e. test/Modules) and name them for the standard version (i.e. cxx20-N-M-exO.cpp) .. because

  • the details of the examples do change from one version of the standard to the next, because of changes in the normative text
  • some of the examples illustrate more than one bullet point
  • it makes it easier to find them for someone who is starting work on this area.

(not a big deal, but now we have some on one place and some in another - which is why I had missed that you already had one of the cases)

I will rebase my changes on this - and I have a patch for the 10.5 PMF issues (under test),

I think it is helpful to collect the standard examples into one place (i.e. test/Modules) and name them for the standard version (i.e. cxx20-N-M-exO.cpp) .. because

  • the details of the examples do change from one version of the standard to the next, because of changes in the normative text
  • some of the examples illustrate more than one bullet point
  • it makes it easier to find them for someone who is starting work on this area.

(not a big deal, but now we have some on one place and some in another - which is why I had missed that you already had one of the cases)

I will rebase my changes on this - and I have a patch for the 10.5 PMF issues (under test),

I prefer to put them under test/CXX/module/xxx. Since it looks like test/CXX is intended to match the standard wording. Another reason is that it looks like we prefer to cite standard in the style of [module.context]p7 instead of 10.6-p7 since the number might change. And you mentioned it too.

some of the examples illustrate more than one bullet point

It doesn't matter since the example it self would live in a certain paragraph.

it makes it easier to find them for someone who is starting work on this area.

I feel like it would be helpful to put them under CXX since the tests under modules mixed many things about OC modules and Clang modules. The reason why some tests of the revision lives in test/Modules is might be hard to find the corresponding wording in the standard. But for things which is clearly corresponding to the standard, I always put them in test/CXX.

iains added a comment.Jun 22 2022, 1:37 AM

I think it is helpful to collect the standard examples into one place (i.e. test/Modules) and name them for the standard version (i.e. cxx20-N-M-exO.cpp) .. because

  • the details of the examples do change from one version of the standard to the next, because of changes in the normative text
  • some of the examples illustrate more than one bullet point
  • it makes it easier to find them for someone who is starting work on this area.

(not a big deal, but now we have some on one place and some in another - which is why I had missed that you already had one of the cases)

I prefer to put them under test/CXX/module/xxx. Since it looks like test/CXX is intended to match the standard wording. Another reason is that it looks like we prefer to cite standard in the style of [module.context]p7 instead of 10.6-p7 since the number might change. And you mentioned it too.

What I was referring to is that the content of the example. the bullet point(s) and even whether the example exists could change between c++20 and, say, c++23 ... but as noted this is not a big deal :)

I think it is helpful to collect the standard examples into one place (i.e. test/Modules) and name them for the standard version (i.e. cxx20-N-M-exO.cpp) .. because

  • the details of the examples do change from one version of the standard to the next, because of changes in the normative text
  • some of the examples illustrate more than one bullet point
  • it makes it easier to find them for someone who is starting work on this area.

(not a big deal, but now we have some on one place and some in another - which is why I had missed that you already had one of the cases)

I prefer to put them under test/CXX/module/xxx. Since it looks like test/CXX is intended to match the standard wording. Another reason is that it looks like we prefer to cite standard in the style of [module.context]p7 instead of 10.6-p7 since the number might change. And you mentioned it too.

What I was referring to is that the content of the example. the bullet point(s) and even whether the example exists could change between c++20 and, say, c++23 ... but as noted this is not a big deal :)

Yeah, it shouldn't be a big problem : )

rsmith accepted this revision.Jun 28 2022, 1:51 PM

A few comments, but they're all minor things or FIXMEs. I'm happy for this to land once they're addressed.

clang/include/clang/AST/DeclBase.h
229
clang/include/clang/Sema/Lookup.h
360–361
373–374
377
clang/lib/Sema/SemaLookup.cpp
1634–1665

Consider adding a Sema::hasAcceptableDefaultArgument to remove the duplicated dispatch here.

1796

Please rename With back to Within.

1895–1962

This function is assuming that any declaration in the ASTContext is in a translation unit on which we have an interface dependency. That's not going to be true in general (for example, using -fmodule-file you can load AST files on which there is no interface dependency, and clang-as-a-library users might even build an ASTContext containing the entire program), but seems good enough for now. Please add a FIXME (eg: "FIXME: Return false if we don't have an interface dependency on the translation unit containing D.").

1898–1907

The any_of here doesn't seem right to me for two reasons:

  1. The rest of this function is answering the question, "is this particular declaration reachable?", not "is any declaration of this entity reachable?"
  2. An entity should be reachable if any declaration of it is. Adding a declaration in a module map module should not cause other declarations to become less reachable.
1926

I don't think we should need the second check here: a declaration that's discarded in the GMF should be module-private, per your comment change on ModueOwnershipKind.

1947

It looks like this change is treating implementation partitions as being reachable only within the same module (see a few lines above: we say anything in the same module is reachable). That's the desirable behavior -- the only reason the standard allows implementation partitions to be reachable anywhere else is because the desirable behavior is harder to implement. So I'm happy with the current approach.

2076–2080

Yes, I'm fine with having FIXMEs for things that don't work yet -- so long as we don't regress things (particularly, so long as we don't regress module map module support, which some of our users are heavily relying on).

I agree that the wording here is not capturing the rules properly; I've mailed the CWG reflector and it sounds like Davis is going to fix that :)

3859

Hm, this might actually be right as-is: because we don't merge lookup results in the decl context lookup set if they come from different modules, we'll consider a friend once for each module that contains a friend declaration, and so we'll consider each class definition that contains a friend declaration here.

I think there's still something subtly wrong here: if there's a merged definition of D that is reachable, then the friend declaration should be considered. If you don't want to handle that here (it might be hard to come up with a testcase), please add a FIXME.

clang/lib/Serialization/ASTReaderDecl.cpp
625–626

Please don't include a default here. We want the compiler to tell us if a new enumerator is added and it isn't handled here.

This revision is now accepted and ready to land.Jun 28 2022, 1:51 PM
ChuanqiXu updated this revision to Diff 440847.Jun 28 2022, 9:30 PM
ChuanqiXu marked 15 inline comments as done.

Address comments.

A few comments, but they're all minor things or FIXMEs. I'm happy for this to land once they're addressed.

Thanks for reviewing!

clang/lib/Sema/SemaLookup.cpp
2076–2080

Thanks! I believe it wouldn't be a regression.

This revision was landed with ongoing or failed builds.Jun 28 2022, 9:50 PM
This revision was automatically updated to reflect the committed changes.