Superficial support for template modifier on symbolic bindings.
#4948
Conversation
Change parse tree from `template (T:! type)` to `(template T):! type`, so that we have information about whether a binding is a template binding available when forming the representation of the binding pattern. This incidentally fixes a bug that we would accept `template addr A:! B` instead of the intended `addr template A:! B`. Track whether a symbolic binding is a template binding on the `EntityName` object. I'm borrowing a bit from the `CompileTimeBindIndex` for this in order to avoid making `EntityName`s larger. Longer-term, we should think about using a different representation for symbolic bindings, to avoid including these fields in all `EntityName`s, but that's out of scope for this change. So far, template bindings are treated as having the same phase as checked bindings, but that will change in a future PR.
| (is_generic ? context.scope_stack().AddCompileTimeBinding() | ||
| : SemIR::CompileTimeBindIndex::None) | ||
| .index, |
There was a problem hiding this comment.
Would it make sense to switch to a constructor, to encapsulate away the .index?
There was a problem hiding this comment.
Encapsulated into a distinct Add function on the value store. (This gets us slightly closer to using distinct storage for this case, and also keeps the field names for the remaining cases.)
There was a problem hiding this comment.
Sure, SG, particularly since we're talking about refactoring anyways.
| // The bind_index() value, unwrapped so it can be stored in a bit-field. | ||
| int32_t bind_index_value : 31 = CompileTimeBindIndex::None.index; | ||
| // Whether this binding is a template parameter. | ||
| bool is_template : 1 = false; |
There was a problem hiding this comment.
Since we're spending time on bit packing, would it make sense to static_assert on sizeof(EntityName)?
There was a problem hiding this comment.
The hashing library will assert if it contains any padding because its check for unique object representations will fail. (We're not caring that much about the size here because we're wasting one id per EntityName on names that aren't symbolic bindings...)
toolchain/sem_ir/entity_name.h
Outdated
| // The index for a compile-time binding. Invalid for a runtime binding, or | ||
| // for a symbolic binding, like `.Self`, that does not correspond to a generic | ||
| // parameter (and therefore has no index). |
There was a problem hiding this comment.
Sorry, I'm making suggestions even though this is pre-existing.
| // The index for a compile-time binding. Invalid for a runtime binding, or | |
| // for a symbolic binding, like `.Self`, that does not correspond to a generic | |
| // parameter (and therefore has no index). | |
| // The index for a compile-time binding. Invalid for runtime bindings. Also invalid | |
| // for a symbolic binding, like `.Self`, that does not correspond to a generic | |
| // parameter (and therefore has no index). |
But, invalid or None?
Maybe (more changes):
| // The index for a compile-time binding. Invalid for a runtime binding, or | |
| // for a symbolic binding, like `.Self`, that does not correspond to a generic | |
| // parameter (and therefore has no index). | |
| // The index for a compile-time binding. `None` for runtime bindings, and | |
| // also for symbolic bindings that don't correspond to a generic parameter | |
| // (and therefore have no index), such as `.Self`. |
There was a problem hiding this comment.
Made even more changes :)
|
LG except for the tests. :) |
Change parse tree from
template (T:! type)to(template T):! type, so that we have information about whether a binding is a template binding available when forming the representation of the binding pattern. This incidentally fixes a bug that we would accepttemplate addr A:! Binstead of the intendedaddr template A:! B.Track whether a symbolic binding is a template binding on the
EntityNameobject. I'm borrowing a bit from theCompileTimeBindIndexfor this in order to avoid makingEntityNames larger. Longer-term, we should think about using a different representation for symbolic bindings, to avoid including these fields in allEntityNames, but that's out of scope for this change.So far, template bindings are treated as having the same phase as checked bindings, but that will change in a future PR.