rsmith added a comment.
Thanks for working on this!
================
Comment at: clang/include/clang/AST/DeclBase.h:228
+ /// This declaration has an owninig module, and is visible to lookups
+ /// that occurs within that module. And it is reachable to other module
----------------
================
Comment at: clang/include/clang/AST/DeclBase.h:230
+ /// that occurs within that module. And it is reachable to other module
+ /// when the owninig module is imported.
+ ReachableWhenImported,
----------------
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1589
+ llvm::SmallVectorImpl<Module *> *Modules,
+ bool RequireReachable = false) {
if (!D->hasDefaultArgument())
----------------
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.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1596-1599
+ if (S.isVisible(D))
+ return true;
+ if (RequireReachable && S.hasReachableDeclaration(D))
+ return true;
----------------
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)`
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1625-1627
+ // It is meaningless to talk about reachable if we are not in C++20 modules.
+ if (!getLangOpts().CPlusPlusModules)
+ return hasVisibleDefaultArgument(D, Modules);
----------------
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.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1650-1654
if (S.isVisible(R))
return true;
+ if (RequireReachable && S.hasReachableDeclaration(R))
+ return true;
----------------
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.)
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1669-1734
bool Sema::hasVisibleExplicitSpecialization(
const NamedDecl *D, llvm::SmallVectorImpl<Module *> *Modules) {
return hasVisibleDeclarationImpl(*this, D, Modules, [](const NamedDecl *D) {
if (auto *RD = dyn_cast<CXXRecordDecl>(D))
return RD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization;
if (auto *FD = dyn_cast<FunctionDecl>(D))
return FD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization;
----------------
I don't like duplicating this code for the reachable / visible checks. Instead,
please add a parameter for the kind (visible / reachable).
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1831-1832
+ // case, the global module fragment shouldn't own an AST File.
+ if (M->isGlobalModule() && M->getASTFile())
+ return false;
+
----------------
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.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:2000-2004
+ // Class and enumeration member names can be found by name lookup in any
+ // context in which a definition of the type is reachable.
+ if (auto *ECD = dyn_cast<EnumConstantDecl>(ND))
+ return getSema().hasReachableDeclaration(
+ cast<NamedDecl>(ECD->getDeclContext()));
----------------
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.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:2014
+ // context in which a definition of the type is reachable.
+ return getSema().hasReachableDeclaration(
+ cast<RecordDecl>(ND->getDeclContext()));
----------------
We should look for a reachable definition, not a reachable declaration.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:3754
+ // reachable definition in the set of associated entities,
+ if (AssociatedClasses.count(RD) && hasReachableDeclaration(D)) {
Visible = true;
----------------
We should look for a reachable definition here, not a reachable declaration.
================
Comment at: clang/lib/Sema/SemaModule.cpp:299
auto *TU = Context.getTranslationUnitDecl();
- TU->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ModulePrivate);
+ if (getLangOpts().CPlusPlusModules)
+ TU->setModuleOwnershipKind(
----------------
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.
================
Comment at: clang/lib/Sema/SemaTemplate.cpp:10990
llvm::SmallVector<Module *, 8> Modules;
+ bool CheckReachability = false;
----------------
Use the `enum` here.
================
Comment at: clang/lib/Sema/SemaType.cpp:8618
+ bool OnlyNeedComplete) {
+ bool IsVisible = hasVisibleDefinition(D, Suggested, OnlyNeedComplete);
+
----------------
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).
================
Comment at: clang/lib/Sema/SemaType.cpp:8632-8634
+ Module *M = D->getOwningModule();
+ if (M && M->isModuleMapModule())
+ return false;
----------------
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.
================
Comment at: clang/lib/Sema/SemaType.cpp:8636-8639
+ // If D isn't from AST file, it implies that D appears in the same TU.
+ // So it should be reachable.
+ if (!D->isFromASTFile())
+ return true;
----------------
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`.
================
Comment at: clang/lib/Sema/SemaType.cpp:8668-8671
+ // Since the standard leaves space for compilers to decide whether the
+ // additional translation unit is reachable, we treat all translation unit as
+ // reachable here to ease the implementation. This strategy would surprise
+ // user less and save compilation time.
----------------
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.
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:616-618
+ if ((int)ModuleOwnership > 4)
+ llvm::report_fatal_error(
+ "The size of sizeModuleOwnership is larger than 4.\n");
----------------
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113545/new/
https://reviews.llvm.org/D113545
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
