Change LexicalLookup to not resize, due to lack of current need. (#3587)

Per discussion, unqualified lookup should only occur on identifiers that
existed at the time Context was initialized. As a consequence, the
resize logic in LexicalLookup may not be necessary. Even considering
metaprogramming, if metaprogramming is restricted to qualified name
lookup, it may not be necessary in the future. Support should be easy to
add if we need it. Trying to clearly document in the CHECK message what
the rationale is.

---------

Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This commit is contained in:
Jon Ross-Perkins
2024-01-11 18:43:31 +00:00
committed by GitHub
co-authored by Richard Smith
parent dc75295a72
commit 52037436eb
3 changed files with 13 additions and 30 deletions
+1 -4
View File
@@ -348,12 +348,9 @@ class Context {
// Directly expose SemIR::File data accessors for brevity in calls.
// Use `lexical_lookup().AddIdentifier` to add entries. `identifiers()` is
// const to discourage misuse.
auto identifiers() -> const StringStoreWrapper<IdentifierId>& {
auto identifiers() -> StringStoreWrapper<IdentifierId>& {
return sem_ir().identifiers();
}
auto ints() -> ValueStore<IntId>& { return sem_ir().ints(); }
auto reals() -> ValueStore<RealId>& { return sem_ir().reals(); }
auto string_literal_values() -> StringStoreWrapper<StringLiteralValueId>& {
+1 -2
View File
@@ -57,8 +57,7 @@ static auto CopyNameFromImportIR(Context& context,
if (auto import_identifier_id = import_name_id.AsIdentifierId();
import_identifier_id.is_valid()) {
auto name = import_sem_ir.identifiers().Get(import_identifier_id);
return SemIR::NameId::ForIdentifier(
context.lexical_lookup().AddIdentifier(name));
return SemIR::NameId::ForIdentifier(context.identifiers().Add(name));
}
return import_name_id;
}
+11 -24
View File
@@ -29,36 +29,23 @@ class LexicalLookup {
ScopeIndex scope_index;
};
explicit LexicalLookup(StringStoreWrapper<IdentifierId>& identifiers)
: identifiers_(&identifiers),
lookup_(identifiers_->size() + SemIR::NameId::NonIndexValueCount) {}
~LexicalLookup() {
CARBON_CHECK(lookup_.size() ==
identifiers_->size() + SemIR::NameId::NonIndexValueCount)
<< lookup_.size() << " must match " << identifiers_->size() << " + "
<< SemIR::NameId::NonIndexValueCount
<< "; something may have been added incorrectly";
}
// Handles both adding the identifier and resizing lookup_ to accommodate the
// new entry. `identifiers().Add` must not be called directly once checking
// has begun.
auto AddIdentifier(llvm::StringRef name) -> IdentifierId {
auto id = identifiers_->Add(name);
// Bear in mind that Add was not guaranteed to actually change the size.
lookup_.resize(identifiers_->size() + SemIR::NameId::NonIndexValueCount);
return id;
}
explicit LexicalLookup(const StringStoreWrapper<IdentifierId>& identifiers)
: lookup_(identifiers.size() + SemIR::NameId::NonIndexValueCount) {}
// Returns the lexical lookup results for a name.
auto Get(SemIR::NameId name_id) -> llvm::SmallVector<Result, 2>& {
return lookup_[name_id.index + SemIR::NameId::NonIndexValueCount];
size_t index = name_id.index + SemIR::NameId::NonIndexValueCount;
CARBON_CHECK(index < lookup_.size())
<< "An identifier was added after the Context was initialized. "
"Currently, we expect that new identifiers will never be used with "
"lexical lookup (they're added for things like detecting name "
"collisions in imports). That might change with metaprogramming: if "
"it does, we may need to start resizing `lookup_`, either on each "
"identifier addition or in Get` where this CHECK currently fires.";
return lookup_[index];
}
private:
StringStoreWrapper<IdentifierId>* identifiers_;
// Maps identifiers to name lookup results.
// TODO: Consider TinyPtrVector<Result> or similar. For now, use a small size
// of 2 to cover the common case.