Opt: Remove useless *Ident indirections in the IR model. #5092
Conversation
9b9a67b to
99cfc45
Compare
| def selfRef(implicit pos: ir.Position) = | ||
| js.VarRef(selfName)(jstpe.AnyType) | ||
| js.VarRef(selfIdent.name)(jstpe.AnyType) |
There was a problem hiding this comment.
IIUC this will change the pos of the synthetic this from pos of the anon class creation to the pos of the definition using it (fields / props).
IMO this is acceptable (or even desirable).
| @@ -1101,7 +1101,7 @@ abstract class GenJSCode[G <: Global with Singleton](val global: G) | |||
| js.JSNew(jsSuperClassRef, args) | |||
| } | |||
|
|
|||
| val selfVarDef = js.VarDef(selfName, thisOriginalName, jstpe.AnyType, mutable = false, newTree) | |||
| val selfVarDef = js.VarDef(selfIdent, thisOriginalName, jstpe.AnyType, mutable = false, newTree) | |||
There was a problem hiding this comment.
On the flipside: This will not change the position of the this ident to the position of the super call.
There was a problem hiding this comment.
Indeed. I added a .copy() to get the correct position here.
| @@ -1088,7 +1088,7 @@ object Trees { | |||
|
|
|||
| // Atomic expressions | |||
|
|
|||
| sealed case class VarRef(ident: LocalIdent)(val tpe: Type)( | |||
| sealed case class VarRef(name: LocalName)(val tpe: Type)( | |||
There was a problem hiding this comment.
Note: I have briefly considered whether VarRef should extend LocalIdent (or even whether we should remove LocalIdent altogether and use VarRef instead.
I do not think we should do this:
- VarDef / ParamDef could lose their
vtpe/ptpefield, but it feels unnatural to have the types in the name. - Other trees
ForIncurrently do not require explicit types, so we'd force an unnecessaryAnyTypethere (which we'd have to IR Check).
| @@ -450,7 +450,7 @@ private[emitter] class FunctionEmitter(sjsGen: SJSGen) { | |||
| implicit pos: Position): js.Ident = { | |||
|
|
|||
| val recIdent = (tree.record: @unchecked) match { | |||
| case VarRef(ident) => transformLocalVarIdent(ident) | |||
| case VarRef(name) => transformLocalVarIdent(name) | |||
There was a problem hiding this comment.
I don't think this is correct. transformLocalVarIdent now takes the record select position, not the position of the var ref.
To prevent this, consider transformVarRef? (IDK if this matters in practice given how the optimizer emits recordselects, but it still seems wrong 🤷 )
There was a problem hiding this comment.
Ah, good catch! I turned it into a transformLocalVarRefIdent(varRef: VarRef): js.Ident to avoid that issue. There is only one call site where we actually wrap it in a js.VarRef, so a transformLocalVarRef(VarRef): js.VarRef would not have helped per se.
Yes. Even then, there is little use for a position on the label. It doesn't hold a value, so things like hovering over the code and other position-related operations won't have anything to show anyway. |
The only point of a `LocalIdent` is to keep track of a position in addition to the local name. However, in a `VarRef`, the position should always be the same as the `VarRef` itself. Storing a separate instance of `LocalIdent` is therefore wasteful.
The position of labels is always irrelevant, as they are synthetic. Therefore, storing them through `LabelIdent`s is wasteful.
99cfc45 to
2ba9ea7
Compare
And port the changes that were made to the compiler backend. Most nobably, the changes from the following upstream PRs: * Introduce non-nullable reference types in the IR. scala-js/scala-js#5018 * Opt: Remove useless *Ident indirections in the IR model. scala-js/scala-js#5092 * Merge the IR node This into VarRef, with a magic LocalName. scala-js/scala-js#5090 * Merge several IR nodes into UnaryOp. scala-js/scala-js#5088 * Restrict usages of StoreModule even further. scala-js/scala-js#5059 * Allow Return arg to be a void if the target Labeled is a void. scala-js/scala-js#5074 * Rename NoType to VoidType and print it as "void". scala-js/scala-js#5061
And port the changes that were made to the compiler backend. Most notably, the changes from the following upstream PRs: * Introduce non-nullable reference types in the IR. scala-js/scala-js#5018 * Opt: Remove useless *Ident indirections in the IR model. scala-js/scala-js#5092 * Merge the IR node This into VarRef, with a magic LocalName. scala-js/scala-js#5090 * Merge several IR nodes into UnaryOp. scala-js/scala-js#5088 * Restrict usages of StoreModule even further. scala-js/scala-js#5059 * Allow Return arg to be a void if the target Labeled is a void. scala-js/scala-js#5074 * Rename NoType to VoidType and print it as "void". scala-js/scala-js#5061
And port the changes that were made to the compiler backend. Most notably, the changes from the following upstream PRs: * scala-js/scala-js#5018 * scala-js/scala-js#5092 * scala-js/scala-js#5090 * scala-js/scala-js#5088 * scala-js/scala-js#5059 * scala-js/scala-js#5074 * scala-js/scala-js#5061
No description provided.