iains added a comment.
@chuanqiXu, thanks for reviewing - but it seems I need to find the right
direction before dealing with the details.
@rsmith - So here is a revised strategy - it is incomplete, and the code
contains debug - so I am posting ****only**** for a review of the direction
taken.
I first tried your suggested `ReachableIfUsed` directly - but that became
somewhat tangled because, as you noted, `used` is not sufficient - we mark
things as used outside of the module purview.
So.
I added `ModuleUnreachable` which is set to be the default module ownership for
the GMF.
When we are in module purview and we mark a decl as used or referenced we reset
that ownership to visible.
So Sema now has thin wrappers over setIsUsed/markUsed/setReferenced that then
handle the case that the decl is marked as `ModuleUnreachable`.
Then comes the **horrible** bit; AFAICT we then have to walk the decl we just
marked to determine if that makes other decls similarly visible - I've made a
partial implementation of this (it just covers function decls - which is enough
to run the example from the standard).
^^^ this is what I need review of direction-wise .. I cannot at present see a
simpler way to do it - or even a reasonable way to cache used-ness dependencies
in the GMF.
Any present, I've elected to stream the `ModuleUnreachable` state which is then
used to ignore decls in lookup.
It seems harder than I'd expected to 'just not stream` those decls
- although that would be what I'd assume to be the motivation for this facility
- reducing the size of PCMs for cases like:
module;
#include <something>
module Foo;
....
suggestions most welcome - if the direction is OK, then the
`markReachableGMFDecls` function needs to be completed before reviewing the
details.
================
Comment at: clang/lib/Sema/Sema.cpp:1154-1155
+
+ for (auto *D : WorkList)
+ DC->removeDecl(D);
+ }
----------------
rsmith wrote:
> rsmith wrote:
> > Please don't remove the declarations from the `DeclContext`; `removeDecl`
> > is inefficient, not well-tested, and not really compatible with the Clang
> > philosophy of AST fidelity. For example, this will be very problematic for
> > tooling that wants to inspect the translation unit after compilation.
> >
> > As an alternative that should also fix the issue with checking `isUsed`,
> > how would you feel about this:
> >
> > * Create a new `ModuleOwnershipKind`, say `ReachableIfUsed`, and set that
> > as the ownership kind for the TU when parsing the global module fragment so
> > it gets inherited into everything we parse in that region. This'll mean
> > that `NextInContextAndBits` needs an extra bit for the ownership field, but
> > I think `Decl` is 8-byte-aligned so that seems fine.
> > * When a declaration is being marked as used, check if its ownership kind
> > is `ReachableIfUsed` and if so change it to `ModulePrivate`.
> >
> > That should be enough to get the desired semantic effect, and would allow
> > us to get the improved diagnostic experience that @ChuanqiXu asked about.
> > As a potentially separate step, we can teach the ASTWriter code to
> > (optionally) skip declarations that are `ReachableIfUsed` (and similarly
> > for internal linkage declarations in module interfaces, function bodies for
> > non-inline functions, and anything else we're allowed to omit).
> > When a declaration is being marked as used, check if its ownership kind is
> > ReachableIfUsed and if so change it to ModulePrivate.
>
> We'd need something a little more subtle, such as checking whether the module
> ownership kind of the translation unit is no longer `ReachableIfUsed`, to
> detect whether we've already left the global module fragment at the point of
> use. Maybe there's somewhere in `Sema` that marks declarations as used that
> we can put this logic in instead?
> > When a declaration is being marked as used, check if its ownership kind is
> > ReachableIfUsed and if so change it to ModulePrivate.
>
> We'd need something a little more subtle, such as checking whether the module
> ownership kind of the translation unit is no longer `ReachableIfUsed`, to
> detect whether we've already left the global module fragment at the point of
> use. Maybe there's somewhere in `Sema` that marks declarations as used that
> we can put this logic in instead?
Yes, what I was doing was way too simplistic - please see the top-level comment
now on the revised direction.
================
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'}}
----------------
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.
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
