Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dyno: handle shadow scopes for 'private use' #21551

Merged
merged 5 commits into from
Feb 9, 2023
Merged

dyno: handle shadow scopes for 'private use' #21551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
faeeaf0
Add shadow scopes to VisibilitySymbols & stringify
mppf Feb 8, 2023
6c30ac4
Small cleanups in call-init-deinit.cpp
mppf Feb 8, 2023
7b7c72f
Adjust doLookupInScope & friends for shadow scopes
mppf Feb 8, 2023
144b7e7
Add some testing for scope resolution improvements
mppf Feb 8, 2023
9ae85f9
Improve comments for 'enum ShadowScope'
mppf Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 67 additions & 13 deletions frontend/include/chpl/resolution/scope-types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,14 +417,7 @@ class Scope {
}
}

void stringify(std::ostream& ss, chpl::StringifyKind stringKind) const {
ss << "Scope ";
ss << tagToString(tag());
ss << " ";
id().stringify(ss, stringKind);
ss << " ";
ss << std::to_string(numDeclared());
}
void stringify(std::ostream& ss, chpl::StringifyKind stringKind) const;

/// \cond DO_NOT_DOCUMENT
DECLARE_DUMP;
Expand Down Expand Up @@ -478,12 +471,46 @@ class VisibilitySymbols {
CONTENTS_EXCEPT,
};

/** Indicating which shadow scope level to use, if any */
enum ShadowScope {
/**
`REGULAR_SCOPE` indicates that no shadow scope is used for the symbols
brought in by this VisibilitySymbols. In other words, the symbols
brought in are at the same scope level as a variable declared next to
the `use` / `import`. `REGULAR_SCOPE` is the shadow scope level
used for:

* `public use`
* `import` aka `private import`
* `public import`
*/
REGULAR_SCOPE = 0,

// 'private use' aka just 'use' introduces 2 shadow scopes
// that appear to be between the module and any parent symbols

/**
`SHADOW_SCOPE_ONE` indicates a shadow scope just outside of
`REGULAR_SCOPE` but before `SHADOW_SCOPE_TWO`. This level is
used for the module contents brought in by a non-public `use`.
*/
SHADOW_SCOPE_ONE = 1,

/**
`SHADOW_SCOPE_TWO` indicates a shadow scope just outside of
`SHADOW_SCOPE_ONE` but before any parent scopes. This level
is used for the module name brought in by a non-public `use`.
*/
SHADOW_SCOPE_TWO = 2,
};

private:
const Scope* scope_; // Scope of the Module etc
// This could technically be an ID but basically
// anything we do with it needs a Scope* anyway.
Kind kind_ = SYMBOL_ONLY;
bool isPrivate_ = true;
int8_t shadowScopeLevel_ = REGULAR_SCOPE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why int8_t instead of ShadowScope?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It saves 4 bytes of memory in each instance here due to the way that C / C++ have enums be int and the way that fields are aligned. If it is a 1 byte type, it can be stored next to the bool, instead of on the next 4-byte aligned word.

We could apply this space optimization in many other places in the uAST but somehow it seemed more important to me in this case, where the enum only stores 3 values. Thanks to our efforts at encapsulation, this field is private, so the type of it is not particularly important to most of the code.


// the names/renames:
// pair.first is the name as declared
Expand All @@ -492,11 +519,17 @@ class VisibilitySymbols {

public:
VisibilitySymbols() { }
VisibilitySymbols(const Scope* scope, Kind kind, bool isPrivate,
VisibilitySymbols(const Scope* scope, Kind kind,
bool isPrivate, ShadowScope shadowScopeLevel,
std::vector<std::pair<UniqueString,UniqueString>> names)
: scope_(scope), kind_(kind), isPrivate_(isPrivate),
: scope_(scope), kind_(kind),
isPrivate_(isPrivate), shadowScopeLevel_(shadowScopeLevel),
names_(std::move(names))
{ }
{
CHPL_ASSERT(shadowScopeLevel == REGULAR_SCOPE ||
shadowScopeLevel == SHADOW_SCOPE_ONE ||
shadowScopeLevel == SHADOW_SCOPE_TWO);
}

/** Return the imported scope */
const Scope* scope() const { return scope_; }
Expand All @@ -507,6 +540,11 @@ class VisibilitySymbols {
/** Return whether or not the imported symbol is private */
bool isPrivate() const { return isPrivate_; }

/** Returns the shadow scope level of the symbols here */
ShadowScope shadowScopeLevel() const {
return (ShadowScope) shadowScopeLevel_;
}

/** Lookup the declared name for a given name
Returns true if `name` is found in the list of renamed names and
stores the declared name in `declared`
Expand All @@ -526,7 +564,8 @@ class VisibilitySymbols {
return scope_ == other.scope_ &&
kind_ == other.kind_ &&
names_ == other.names_ &&
isPrivate_ == other.isPrivate_;
isPrivate_ == other.isPrivate_ &&
shadowScopeLevel_ == other.shadowScopeLevel_;
}
bool operator!=(const VisibilitySymbols& other) const {
return !(*this == other);
Expand All @@ -537,6 +576,7 @@ class VisibilitySymbols {
std::swap(kind_, other.kind_);
names_.swap(other.names_);
std::swap(isPrivate_, other.isPrivate_);
std::swap(shadowScopeLevel_, other.shadowScopeLevel_);
}

static bool update(VisibilitySymbols& keep,
Expand All @@ -551,6 +591,12 @@ class VisibilitySymbols {
p.second.mark(context);
}
}

void stringify(std::ostream& ss, chpl::StringifyKind stringKind) const;

/// \cond DO_NOT_DOCUMENT
DECLARE_DUMP;
/// \endcond DO_NOT_DOCUMENT
};

/**
Expand Down Expand Up @@ -579,9 +625,13 @@ class ResolvedVisibilityScope {
/** Add a visibility clause */
void addVisibilityClause(const Scope* scope, VisibilitySymbols::Kind kind,
bool isPrivate,
VisibilitySymbols::ShadowScope shadowScopeLevel,
std::vector<std::pair<UniqueString,UniqueString>> n)
{
visibilityClauses_.emplace_back(scope, kind, isPrivate, std::move(n));
auto elt = VisibilitySymbols(scope, kind,
isPrivate, shadowScopeLevel,
std::move(n));
visibilityClauses_.push_back(std::move(elt));
}

bool operator==(const ResolvedVisibilityScope& other) const {
Expand All @@ -601,6 +651,10 @@ class ResolvedVisibilityScope {
sym.mark(context);
}
}
void stringify(std::ostream& ss, chpl::StringifyKind stringKind) const;
/// \cond DO_NOT_DOCUMENT
DECLARE_DUMP;
/// \endcond DO_NOT_DOCUMENT
};

/*
Expand Down
5 changes: 2 additions & 3 deletions frontend/lib/resolution/call-init-deinit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ static bool isRecordLike(const Type* t) {
}

static bool typeNeedsInitDeinitCall(const Type* t) {
if (t->isUnknownType() || t->isErroneousType()) {
if (t == nullptr || t->isUnknownType() || t->isErroneousType()) {
// can't do anything with these
return false;
} else if (t->isPrimitiveType() || t->isBuiltinType() || t->isCStringType() ||
Expand Down Expand Up @@ -607,8 +607,7 @@ void CallInitDeinit::resolveMoveInit(const AstNode* ast,
getTypeGenericity(context, rhsType) != Type::CONCRETE;
// resolve a copy init and a deinit to deinit the temporary
if (lhsGenUnk || rhsGenUnk) {
// TODO: this should not happen
printf("Warning: should not be reached\n");
CHPL_ASSERT(false && "should not be reached");
} else {
resolveCopyInit(ast, rhsAst, lhsType, rhsType,
/* forMoveInit */ true,
Expand Down
Loading