Simplify interface for getting an instruction from a type. #3455
Conversation
toolchain/sem_ir/file.h
Outdated
| @@ -238,6 +239,30 @@ class File : public Printable<File> { | |||
| } | |||
| } | |||
|
|
|||
| // Returns the instruction used to define the specified type. | |||
| auto GetType(TypeId type_id) const -> Inst { | |||
There was a problem hiding this comment.
This feels like it should be GetTypeInst, since GetType would really be types().Get. Similar thoughts on below.
Maybe these could be moved onto insts(), where you could do insts().GetType more naturally? Could that have a reference to types() without creating problems?
There was a problem hiding this comment.
Last time I looked at this, referencing one ValueStore from another was problematic due to File being copyable (and actually copied in practice). But... after the recent changes for imports, we might not be copying Files any more, so maybe this is easier now.
There was a problem hiding this comment.
Yeah, I think the ValueStores already aren't copyable.
There was a problem hiding this comment.
There's also the BumpPtrAllocator
There was a problem hiding this comment.
Moved onto types() , rather than insts(). I think this makes the call sites read a bit better, but I could go either way.
toolchain/sem_ir/file.h
Outdated
| @@ -226,7 +226,8 @@ class File : public Printable<File> { | |||
| complete_types_.push_back(object_type_id); | |||
| } | |||
|
|
|||
| auto GetTypeAllowBuiltinTypes(TypeId type_id) const -> InstId { | |||
| // Returns the ID of the instruction used to define the specified type. | |||
| auto GetTypeInstId(TypeId type_id) const -> InstId { | |||
There was a problem hiding this comment.
As long as you're touching this, maybe types().GetInstId?
|
Note, I'm okay with this as-is if you want to merge, but I do lean towards moving things off File. |
Co-authored-by: Jon Ross-Perkins <[email protected]>
No description provided.