Refactor: Make FieldName a composite of ClassName and SimpleFieldName. #4946
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sjrd
commented
Feb 17, 2024
| * (And no, this is not dead code.) | ||
| */ | ||
| val recordField = recordType.findField(field.name.simpleName) | ||
| val sel = RecordSelect(newQual, SimpleFieldIdent(recordField.name))(recordField.tpe) |
There was a problem hiding this comment.
This snippet is worth paying particular attention to. The change does not alter the logic AFAICT, but it does highlight that there is/was something fishy going on.
There was a problem hiding this comment.
Looks like records only use simple field names. Maybe this will blow up:
class A {
private var x: Int
def foo = x
}
@inline
class B extends A {
private var x: String
}
new B().fooThere was a problem hiding this comment.
I have checked this, and it seems to work fine. What I've tested:
class A {
private var x: Int = 1
def foo = x
}
@inline
class B extends A {
private var x: String = "f"
def bar = x
}
object HelloWorld {
@noinline
def p(x: Any) = println(x)
def main(args: Array[String]): Unit = {
val b = new B()
p(b.foo)
p(b.bar)
}
}Relevant generated code:
$c_Lhelloworld_HelloWorld$.prototype.main__AT__V = (function(args) {
var x = 0;
var x$1 = null;
x = 1;
x$1 = "f";
this.p__O__V(x);
this.p__O__V(x$1);
});There was a problem hiding this comment.
Weird. Perhaps that specific example does not run into that particular code path.
gzm0
requested changes
Feb 17, 2024
linker/shared/src/main/scala/org/scalajs/linker/backend/emitter/NameGen.scala
Show resolved
Hide resolved
linker/shared/src/main/scala/org/scalajs/linker/backend/emitter/FunctionEmitter.scala
Show resolved
Hide resolved
linker/shared/src/main/scala/org/scalajs/linker/backend/emitter/VarGen.scala
Show resolved
Hide resolved
| @@ -1738,9 +1754,28 @@ object Serializers { | |||
| LabelIdent(readLabelName()) | |||
| } | |||
|
|
|||
| def readSimpleFieldIdent(): SimpleFieldIdent = { | |||
There was a problem hiding this comment.
Unused? Seems like we do not actually ever read records :P
| * (And no, this is not dead code.) | ||
| */ | ||
| val recordField = recordType.findField(field.name.simpleName) | ||
| val sel = RecordSelect(newQual, SimpleFieldIdent(recordField.name))(recordField.tpe) |
There was a problem hiding this comment.
Looks like records only use simple field names. Maybe this will blow up:
class A {
private var x: Int
def foo = x
}
@inline
class B extends A {
private var x: String
}
new B().foo| genFieldNameCache.getOrElseUpdate(name, { | ||
| genName(name.className) + "__f_" + genName(name.simpleName) | ||
| }) | ||
| } |
There was a problem hiding this comment.
I would not cache this for now, to avoid changing performance behavior. (Also see comments RE duplicate caching of simple field names).
Previously, `FieldName` only represented the *simple* name of a field. It was complemented everywhere with the enclosing `ClassName`, for namespacing purposes. We now make `FieldName` a composite, like `MethodName`. It contains a `ClassName` and a `SimpleFieldName`. Structurally, `SimpleFieldName` is the same as the old `FieldName`. This removes the need to pass additional, out-of-band `ClassName`s everywhere a `FieldName` or `FieldIdent` was used. In addition to the readability improvements, this might improve performance. We previously often had to create (temporary) pairs of `(ClassName, FieldName)` as keys of maps. Now, we can directly use the `FieldName`s instead. While the IR names, types and trees are significantly impacted by this change, the `.sjsir` format is unchanged.
61302e8 to
723663b
Compare
gzm0
approved these changes
Feb 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously,
FieldNameonly represented the simple name of a field. It was complemented everywhere with the enclosingClassName, for namespacing purposes.We now make
FieldNamea composite, likeMethodName. It contains aClassNameand aSimpleFieldName. Structurally,SimpleFieldNameis the same as the oldFieldName.This removes the need to pass additional, out-of-band
ClassNames everywhere aFieldNameorFieldIdentwas used.In addition to the readability improvements, this might improve performance. We previously often had to create (temporary) pairs of
(ClassName, FieldName)as keys of maps. Now, we can directly use theFieldNames instead.While the IR names, types and trees are significantly impacted by this change, the
.sjsirformat is unchanged.