iains marked 3 inline comments as done.
iains added a comment.
@ChuanqiXu - I changed the module ownership name to "ModuleDiscardable" -
because, while we are permitted to discard these, we might choose not to (to
give your better diagnostics) - but we still need to treat them as
non-reachable and non-visible. Please could you examine what is happening with
`Modules/explicitly-specialized-template.cpp` where there a multiple error
messages for each (intentionally) failing line .. add the test from the std.
@rsmith
this seems like an awful lot of work that has to be done for every decl we
mark used in the module purview - we cannot even short-cut ones that are not in
the GMF, since the provisions of [module.global.frag] p3.5 seem to allow this
to happen for indirect reaching. I wonder if I misread the std.
I am also wondering what is supposed to happen when an interface makes a type
reachable, but that type is not visible to the implementation .. it seems to
mean that interfaces would have to add using declarations for each such type.
================
Comment at: clang/include/clang/AST/DeclBase.h:240
+ /// GMF is part.
+ ModuleUnreachable
};
----------------
ChuanqiXu wrote:
> Would you like to use the term 'ModuleDiscarded'? My point is that not all
> unreachable declaration in modules are in GMF. And the term `discard` is used
> in standard to describe it. So it looks better.
`ModuleDiscardable` Is better, it says we have permission to discard it (so
that it cannot be used or reached) but allows for the case we elect to keep
them around for better diagnostics. You might want to consider renaming the
wrapper function similarly?
================
Comment at: clang/include/clang/AST/DeclBase.h:647
+ bool isDiscardedInGlobalModuleFragment() const {
+ return getModuleOwnershipKind() == ModuleOwnershipKind::ModuleUnreachable;
+ }
----------------
ChuanqiXu wrote:
> iains wrote:
> > ChuanqiXu wrote:
> > > Maybe we need to check if the owning module is global module or not.
> > The only place that the ownership is `ModuleUnreachable` is in the GMF - so
> > that we do not need to do an additional test.
> >
> It is more robust and clear to add the additional check to me. Since the
> constraint now lives in the comment only. If anyone goes to change it, the
> codes wouldn't complain.
I am very concerned about the amount of work this (GMF decl elision) adds, so
would like to minimise this - what I have done is to add an assert at the point
we might make a change to the ownership that tests to see the decl is in the
fragment we expect.
================
Comment at: clang/lib/Sema/SemaModule.cpp:1009
+ : VD->getASTContext().getUnqualifiedObjCPointerType(VD->getType());
+ T.dump();
+ }
----------------
ChuanqiXu wrote:
> Remove dump
of course, as noted, the patch here is for comment on approach.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126694/new/
https://reviews.llvm.org/D126694
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
