rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
A few comments, but they're all minor things or FIXMEs. I'm happy for this to
land once they're addressed.
================
Comment at: clang/include/clang/AST/DeclBase.h:229
+ /// This declaration has an owning module, and is visible to lookups
+ /// that occurs within that module. And it is reachable to other module
+ /// when the owning module is transitively imported.
----------------
================
Comment at: clang/include/clang/Sema/Lookup.h:360-361
+ /// Determine whether this lookup is permitted to see the declaration.
+ /// Note that the reachable but not visible declaration inhabit at namespace
+ /// is not allowed to be seen during name lookup.
+ ///
----------------
================
Comment at: clang/include/clang/Sema/Lookup.h:373-374
+ /// // Not valid. We couldn't see reachable here.
+ /// // So isAvailableForLookup would return false when we looks
+ /// 'reachable' here.
+ /// // return reachable(43).v;
----------------
================
Comment at: clang/include/clang/Sema/Lookup.h:377
+ /// // Valid. The field name 'v' is allowed during name lookup.
+ /// // So isAvailableForLookup would return true when we looks 'v' here.
+ /// return func().v;
----------------
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1634-1658
bool Sema::hasVisibleDefaultArgument(const NamedDecl *D,
llvm::SmallVectorImpl<Module *> *Modules)
{
if (auto *P = dyn_cast<TemplateTypeParmDecl>(D))
- return ::hasVisibleDefaultArgument(*this, P, Modules);
+ return ::hasAcceptableDefaultArgument(*this, P, Modules,
+ Sema::AcceptableKind::Visible);
if (auto *P = dyn_cast<NonTypeTemplateParmDecl>(D))
+ return ::hasAcceptableDefaultArgument(*this, P, Modules,
----------------
Consider adding a `Sema::hasAcceptableDefaultArgument` to remove the duplicated
dispatch here.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1796
// set of declarations for tags in prototype scope.
- bool VisibleWithinParent;
+ bool AcceptableWithParent;
if (D->isTemplateParameter()) {
----------------
Please rename `With` back to `Within`.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1895
-bool Sema::isVisibleSlow(const NamedDecl *D) {
- return LookupResult::isVisible(*this, const_cast<NamedDecl*>(D));
+bool LookupResult::isReachableSlow(Sema &SemaRef, NamedDecl *D) {
+ assert(!isVisible(SemaRef, D) && "Shouldn't call the slow case.\n");
----------------
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.").
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1898-1907
+ // Filter the use other than C++20 Considering the case we are using clang
+ // module + -std=c++20 combination.
+ if (llvm::any_of(D->redecls(), [](Decl *D) {
+ return D->getOwningModule() &&
+ D->getOwningModule()->isModuleMapModule();
+ }))
+ return false;
----------------
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.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1926
+ // fragment.
+ if (D->isModulePrivate() || D->isDiscardedInGlobalModuleFragment())
+ return false;
----------------
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`.
================
Comment at: clang/lib/Sema/SemaLookup.cpp:1947
+ // DeclModule if it isn't (transitively) imported.
+ if (DeclModule->getTopLevelModule()->isModuleInterfaceUnit())
+ return true;
----------------
ChuanqiXu wrote:
> iains wrote:
> > ChuanqiXu wrote:
> > > iains wrote:
> > > > ChuanqiXu wrote:
> > > > > iains wrote:
> > > > > > I think isModuleInterfaceUnit needs to include implementation
> > > > > > partition units, since they also have a BMI (is
> > > > > > `isInterfaceOrPartition()` the right thing to use here?
> > > > > 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.
> > > >
> > > > 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?
> > > >
> > > > ...
> > > > ```
> > > >
> > > >
> > > >
> > > >
> > > 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.
> > (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.
> >
> >
> 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)
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.
================
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()));
----------------
ChuanqiXu wrote:
> rsmith wrote:
> > 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.
> 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.
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 :)
================
Comment at: clang/lib/Sema/SemaLookup.cpp:3754
+ // reachable definition in the set of associated entities,
+ if (AssociatedClasses.count(RD) && hasReachableDeclaration(D)) {
Visible = true;
----------------
ChuanqiXu wrote:
> rsmith wrote:
> > We should look for a reachable definition here, not a reachable declaration.
> 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.
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.
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:626-627
+
+ default:
+ llvm_unreachable("Unknown ModuleOwnership kind");
+ }
----------------
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.
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
