Copilot commented on code in PR #2186:
URL: https://github.com/apache/groovy/pull/2186#discussion_r2040641694
##########
src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java:
##########
@@ -192,113 +195,130 @@ private void declare(final Variable variable, final
ASTNode context) {
currentScope.putDeclaredVariable(variable);
}
+ private final Map<ClassMemberCacheKey, VariableWrapper> classMemberCache =
new HashMap<>(64);
private Variable findClassMember(final ClassNode node, final String name) {
- final boolean abstractType = node.isAbstract();
-
- for (ClassNode cn = node; cn != null && !ClassHelper.isObjectType(cn);
cn = cn.getSuperClass()) {
- for (FieldNode fn : cn.getFields()) {
- if (name.equals(fn.getName())) return fn;
- }
+ VariableWrapper variableWrapper = classMemberCache.computeIfAbsent(new
ClassMemberCacheKey(name, node), k -> {
+ final ClassNode classNode = k.node;
+ final String memberName = k.name;
+ final boolean abstractType = classNode.isAbstract();
+
+ for (ClassNode cn = classNode; cn != null &&
!ClassHelper.isObjectType(cn); cn = cn.getSuperClass()) {
+ for (FieldNode fn : cn.getFields()) {
+ if (memberName.equals(fn.getName())) return new
VariableWrapper(fn);
+ }
- for (PropertyNode pn : cn.getProperties()) {
- if (name.equals(pn.getName())) return pn;
- }
+ for (PropertyNode pn : cn.getProperties()) {
+ if (memberName.equals(pn.getName())) return new
VariableWrapper(pn);
+ }
- for (MethodNode mn : cn.getMethods()) {
- if ((abstractType || !mn.isAbstract()) &&
name.equals(getPropertyName(mn))) {
- // check for super property before returning a
pseudo-property
- for (PropertyNode pn :
getAllProperties(cn.getSuperClass())) {
- if (name.equals(pn.getName())) return pn;
+ for (MethodNode mn : cn.getMethods()) {
+ if ((abstractType || !mn.isAbstract()) &&
memberName.equals(getPropertyName(mn))) {
+ // check for super property before returning a
pseudo-property
+ for (PropertyNode pn :
getAllProperties(cn.getSuperClass())) {
+ if (memberName.equals(pn.getName())) return new
VariableWrapper(pn);
+ }
+
+ FieldNode fn = new FieldNode(memberName,
mn.getModifiers() & 0xF, ClassHelper.dynamicType(), cn, null);
+ fn.setHasNoRealSourcePosition(true);
+ fn.setDeclaringClass(cn);
+ fn.setSynthetic(true);
+
+ PropertyNode pn = new PropertyNode(fn,
fn.getModifiers(), null, null);
+ pn.putNodeMetaData("access.method", mn);
+ pn.setDeclaringClass(cn);
+ return new VariableWrapper(pn);
}
+ }
- FieldNode fn = new FieldNode(name, mn.getModifiers() &
0xF, ClassHelper.dynamicType(), cn, null);
- fn.setHasNoRealSourcePosition(true);
- fn.setDeclaringClass(cn);
- fn.setSynthetic(true);
-
- PropertyNode pn = new PropertyNode(fn, fn.getModifiers(),
null, null);
- pn.putNodeMetaData("access.method", mn);
- pn.setDeclaringClass(cn);
- return pn;
+ for (ClassNode in : cn.getAllInterfaces()) {
+ FieldNode fn = in.getDeclaredField(memberName);
+ if (fn != null) return new VariableWrapper(fn);
+ PropertyNode pn = in.getProperty(memberName);
+ if (pn != null) return new VariableWrapper(pn);
}
}
- for (ClassNode in : cn.getAllInterfaces()) {
- FieldNode fn = in.getDeclaredField(name);
- if (fn != null) return fn;
- PropertyNode pn = in.getProperty(name);
- if (pn != null) return pn;
- }
- }
+ return new VariableWrapper(null);
Review Comment:
Returning a VariableWrapper wrapping null may lead to ambiguity in cache
lookups. Consider handling cache misses explicitly so that the absence of a
variable is distinguishable without wrapping a null value.
```suggestion
return VariableWrapper.EMPTY;
```
##########
src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java:
##########
@@ -760,4 +780,66 @@ public void visitVariableExpression(final
VariableExpression expression) {
checkVariableContextAccess(variable, expression);
}
}
+
+ private static class VariableWrapper {
+ private final Variable variable;
+ private int accessedCount;
+
+ VariableWrapper(final Variable variable) {
+ this.variable = variable;
+ }
+ }
+
+ private static class ClassMemberCacheKey {
+ private static final int DEFAULT_HASH = -1;
+ private final String name;
+ private final ClassNode node;
+ private int hash = DEFAULT_HASH;
+
+ ClassMemberCacheKey(final String name, final ClassNode node) {
+ this.name = name;
+ this.node = node;
+ }
+
+ @Override
+ public boolean equals(final Object obj) {
+ if (this == obj) return true;
+ if (!(obj instanceof ClassMemberCacheKey)) return false;
+ ClassMemberCacheKey that = (ClassMemberCacheKey) obj;
+ return name.equals(that.name) && node.equals(that.node);
+ }
+
+ @Override
+ public int hashCode() {
+ return DEFAULT_HASH != hash ? hash : (hash = Objects.hash(name,
node));
Review Comment:
Using -1 as DEFAULT_HASH may cause an issue if Objects.hash returns -1,
leading to an incorrect cached hash. Consider choosing a different sentinel
value to avoid potential collisions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@groovy.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
