ChuanqiXu added a comment.
In D126694#3564629 <https://reviews.llvm.org/D126694#3564629>, @iains wrote:
> the first failure is like this:
>
> x-h.h:
> struct A {
> friend int operator+(const A &lhs, const A &rhs) {
> return 0;
> }
> };
>
> X.cpp:
> module;
> #include "x-h.h"
> export module X;
>
> export using ::A;
>
> This does not mark *anything* in the GMF (x-h.h) as 'used', and so everything
> there is unreachable (and hence the fails).
> I.e `export using ::A;` neither marks A as used or referenced.
> I am not sure if we are supposed to special-case this - since
> `https://eel.is/c++draft/module#global.frag-3.6` explicitly says "In this
> determination, it is unspecified .. whether `using-declaration, ... is
> replaced by the declarations they name prior to this determination,`
> so .. not about how to proceed with this one at present;
> edit: but it seems most reasonable to make it behave as if A was itself
> exported.
I highly recommend we should mark A here. Maybe we need other interfaces than
markDeclUsed and setDeclReferenced. If we don't support this, we couldn't use
modules like
https://github.com/alibaba/async_simple/blob/CXX20Modules/third_party_module/asio/asio.cppm.
This manner is important to use C++20 modules before the build system is
ready. Also, I think it is an important tool to implement C++23's std modules.
So we should support it.
================
Comment at: clang/include/clang/AST/DeclBase.h:647
+ bool isDiscardedInGlobalModuleFragment() const {
+ return getModuleOwnershipKind() == ModuleOwnershipKind::ModuleUnreachable;
+ }
----------------
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.
================
Comment at: clang/include/clang/AST/DeclBase.h:240
+ /// GMF is part.
+ ModuleUnreachable
};
----------------
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.
================
Comment at: clang/include/clang/Sema/Sema.h:2273
+ void HandleGMFReachability(Decl *D) {
+ if (D->isModuleUnreachable() && isCurrentModulePurview()) {
+
D->setModuleOwnershipKind(Decl::ModuleOwnershipKind::ReachableWhenImported);
----------------
iains wrote:
> ChuanqiXu wrote:
> > I feel better if we would check if D lives in GMF. (We need to insert a
> > check in isDiscardedInGlobalModuleFragment)
> If the consensus is to add an extra test, OK.
>
> However, as above, specifically to avoid making more and more tests in code
> that is executed very frequently - as the design currently stands, the only
> place that `ModuleUnreachable` is set is in the GMF.
Yeah, my opinion is the same as above. Although it is the design, it is more
semantically clear and robust to add a additional check. I am just afraid it
would confuse and block other readers or contributors.
================
Comment at: clang/lib/AST/TextNodeDumper.cpp:1622
+ if (D->isModuleUnreachable())
+ OS << " ModuleUnreachable";
}
----------------
iains wrote:
> ChuanqiXu wrote:
> > It may be better to keep the consistent style.
> I don't think that is a matter of style `__module_private__` is a keyword
> used elsewhere?
>
> If you look though the file you will see mostly that the printed output does
> not prepend or append underscores.
>
> BTW, similar changes are probably needed in other node printers, this was
> done early to add debug.
Oh, I found `__module_private__ ` is a keyword in clang modules. I didn't
recognize it. Even in this case, I still prefer to keep the style consistently.
I think users would be more comfortable to read consistent symbols. Also I
think it is acceptable to keep `ModuleUnreachable` since it doesn't matter a
lot to me.
================
Comment at: clang/lib/Sema/SemaModule.cpp:978
+void Sema::markReachableGMFDecls(Decl *Orig) {
+
+ if (isa<FunctionDecl>(*Orig)) {
----------------
iains wrote:
> ChuanqiXu wrote:
> > It looks better to add assumption as an assertion .
> what is `D` here?
> `markReachableGMFDecls` is only called in the case that `Orig` is **not**
> discarded (we are marking decls as reachable because they are `used` in the
> interface..
>
Oh, `D` should be `Orig`, my bad. Yeah, I found `markReachableGMFDecls ` is
only called when `Orig` is not discarded. So I suggest to add the assertion to
make it explicit and clear and it could avoid misuse in the future.
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:635-636
- if (ModulePrivate) {
- // Module-private declarations are never visible, so there is no work to
- // do.
+ if (ModulePrivate ||
+ ModuleOwnership == Decl::ModuleOwnershipKind::ModuleUnreachable) {
+ // Module-private and unreachable declarations are never visible, so
----------------
iains wrote:
> ChuanqiXu wrote:
> > Maybe we could make a new bool variable like `ModulePrivate` to keep the
> > style consistent. So that we could write:
> yes, perhaps we could do that - I am wondering if that code can all be
> factored into the switch.
It looks possible.
================
Comment at: clang/test/CXX/basic/basic.scope/basic.scope.namespace/p2.cpp:32
void test_early() {
- in_header = 1; // expected-error {{missing '#include "foo.h"'; 'in_header'
must be declared before it is used}}
- // expected-note@*{{not visible}}
+ in_header = 1; // expected-error {{use of undeclared identifier 'in_header'}}
----------------
iains wrote:
> ChuanqiXu wrote:
> > iains wrote:
> > > ChuanqiXu wrote:
> > > > This error message looks worse. I image I could hear users complaining
> > > > this. (I doesn't say it is your fault since this is specified in
> > > > standard and the standard cases are harder to understand). I want to
> > > > know if this is consistent with GCC?
> > > > This error message looks worse. I image I could hear users complaining
> > > > this. (I doesn't say it is your fault since this is specified in
> > > > standard and the standard cases are harder to understand). I want to
> > > > know if this is consistent with GCC?
> > >
> > > As you say, the standard says "neither reachable nor visible"
> > > if it's not reachable then. we would not have the information that it was
> > > from header foo.h so we cannot form the nicer diagnostic.
> > >
> > > Perhaps we need to invent "reachable for diagnostics" ... which I'd
> > > rather not divert effort to right now.
> > >
> > Maybe we could make a new diagnostic message.
> I do not think it is a matter of a diagnostic message.
>
> If we omit the decl from the BMI (which we are permitted to do if it is
> ModuleUnreachable), then it cannot be found and therefore the information on
> which header it came from would not be available.
>
Your word makes sense.
================
Comment at: clang/test/Modules/cxx20-10-4-ex2.cpp:44
+template<typename T> int use_h() {
+ N::X x; // N::X, N, and :: are decl-reachable from use_h
+ return h((T(), x)); // N::h is not decl-reachable from use_h, but
----------------
There is invisible symbol.
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
