You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by GitBox <gi...@apache.org> on 2021/05/12 12:37:08 UTC
[GitHub] [groovy] paulk-asert opened a new pull request #1578: GROOVY-9632: Java 8 Type Param Annotation Not Generated in Byte Code
paulk-asert opened a new pull request #1578:
URL: https://github.com/apache/groovy/pull/1578
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] danielsun1106 merged pull request #1578: GROOVY-9632: Java 8 Type Param Annotation Not Generated in Byte Code
Posted by GitBox <gi...@apache.org>.
danielsun1106 merged pull request #1578:
URL: https://github.com/apache/groovy/pull/1578
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] danielsun1106 commented on a change in pull request #1578: GROOVY-9632: Java 8 Type Param Annotation Not Generated in Byte Code
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on a change in pull request #1578:
URL: https://github.com/apache/groovy/pull/1578#discussion_r631482165
##########
File path: src/main/java/org/codehaus/groovy/ast/ClassNode.java
##########
@@ -1420,6 +1421,16 @@ public void setUsingGenerics(boolean usesGenerics) {
this.usesGenerics = usesGenerics;
}
+ public ClassNode getPlainPrimitiveReference() {
+ ClassNode n = new ClassNode(name, modifiers, superClass, null, null);
+ n.isPrimaryNode = false;
+ n.setRedirect(redirect());
+ if (isArray()) {
+ n.componentType = redirect().getComponentType();
+ }
+ return n;
+ }
+
public ClassNode getPlainNodeReference() {
if (ClassHelper.isPrimitiveType(this)) return this;
ClassNode n = new ClassNode(name, modifiers, superClass, null, null);
Review comment:
We could remove the duplicated code by invoking the method:
`return getPlainPrimitiveReference()`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] danielsun1106 commented on pull request #1578: GROOVY-9632: Java 8 Type Param Annotation Not Generated in Byte Code
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on pull request #1578:
URL: https://github.com/apache/groovy/pull/1578#issuecomment-841805112
Merged. Thanks!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] danielsun1106 commented on pull request #1578: GROOVY-9632: Java 8 Type Param Annotation Not Generated in Byte Code
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on pull request #1578:
URL: https://github.com/apache/groovy/pull/1578#issuecomment-841612482
+1
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] danielsun1106 commented on pull request #1578: GROOVY-9632: Java 8 Type Param Annotation Not Generated in Byte Code
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on pull request #1578:
URL: https://github.com/apache/groovy/pull/1578#issuecomment-841805112
Merged. Thanks!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] danielsun1106 merged pull request #1578: GROOVY-9632: Java 8 Type Param Annotation Not Generated in Byte Code
Posted by GitBox <gi...@apache.org>.
danielsun1106 merged pull request #1578:
URL: https://github.com/apache/groovy/pull/1578
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] danielsun1106 commented on a change in pull request #1578: GROOVY-9632: Java 8 Type Param Annotation Not Generated in Byte Code
Posted by GitBox <gi...@apache.org>.
danielsun1106 commented on a change in pull request #1578:
URL: https://github.com/apache/groovy/pull/1578#discussion_r631483818
##########
File path: src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java
##########
@@ -873,7 +873,7 @@ public ExpressionStatement visitResource(final ResourceContext ctx) {
@Override
public List<ClassNode> visitCatchType(final CatchTypeContext ctx) {
if (!asBoolean(ctx)) {
- return Collections.singletonList(ClassHelper.OBJECT_TYPE);
+ return Collections.singletonList(ClassHelper.OBJECT_TYPE.getPlainNodeReference());
Review comment:
Out of curiosity, why should we invoke `getPlainNodeReference()` on `OBJECT_TYPE` instead of using `OBJECT_TYPE` directly? Also, I find many similar changes in the PR, so I assume lots of changes from `==` to `equals(...)` is caused by the change too.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on a change in pull request #1578: GROOVY-9632: Java 8 Type Param Annotation Not Generated in Byte Code
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on a change in pull request #1578:
URL: https://github.com/apache/groovy/pull/1578#discussion_r631739772
##########
File path: src/main/java/org/codehaus/groovy/ast/ClassNode.java
##########
@@ -1420,6 +1421,16 @@ public void setUsingGenerics(boolean usesGenerics) {
this.usesGenerics = usesGenerics;
}
+ public ClassNode getPlainPrimitiveReference() {
+ ClassNode n = new ClassNode(name, modifiers, superClass, null, null);
+ n.isPrimaryNode = false;
+ n.setRedirect(redirect());
+ if (isArray()) {
+ n.componentType = redirect().getComponentType();
+ }
+ return n;
+ }
+
public ClassNode getPlainNodeReference() {
if (ClassHelper.isPrimitiveType(this)) return this;
ClassNode n = new ClassNode(name, modifiers, superClass, null, null);
Review comment:
That would be possible. Let's see first if others feel the method makes sense. We could remove the new method and just always call `getPlainNodeReference()` if we removed the primitive check line. I just felt that might be more likely to break existing external code.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [groovy] paulk-asert commented on a change in pull request #1578: GROOVY-9632: Java 8 Type Param Annotation Not Generated in Byte Code
Posted by GitBox <gi...@apache.org>.
paulk-asert commented on a change in pull request #1578:
URL: https://github.com/apache/groovy/pull/1578#discussion_r631492953
##########
File path: src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java
##########
@@ -873,7 +873,7 @@ public ExpressionStatement visitResource(final ResourceContext ctx) {
@Override
public List<ClassNode> visitCatchType(final CatchTypeContext ctx) {
if (!asBoolean(ctx)) {
- return Collections.singletonList(ClassHelper.OBJECT_TYPE);
+ return Collections.singletonList(ClassHelper.OBJECT_TYPE.getPlainNodeReference());
Review comment:
Yes, it is a change in numerous previous assumptions. It would be good for others to think the change through too. It is somewhat of a compromise to get the needed functionality and minimise my perceived potential breakages in existing frameworks and existing code. Basically, for this:
```
@Anno1
def x = 'foo'
@Anno2
def y = 42
```
The type of the field is `Object`. If `Anno1` and `Anno2` are field annotations, they are stored on the `FieldNode` itself and all is well. It is trickier if `Anno1` and `Anno2` are type annotations. We can either try to stored them along with other annotations in the `FieldNode` and extract them out when writing bytecode. Indeed that is what we did for our initial implementation for classes and methods/method parameters.
I in fact started heading further down this path when adding support for generic types. I made `GenericsType` extend `AnnotatedNode` but then you need to also account for annotations on the generic type bounds and later for casts, new expressions, catch target expressions and the list goes on. So I headed down the current path and store the type annotations on the types. Again there are implementation details which impacted so choices I made, e.g. we currently get normal annotations from the redirect classnode if it exists. Adding a separate list of type annotations seemed the safest path forward. So going back to `getPlainNodeReference`, we obviously don't want `Anno1` and `Anno2` to be present in all places where `OBJECT` occurs. For memory efficiency we currently share OBJECT, primitives and void throughout our data structures as pseudo immutable types which we can't do if type annotations are present.
I have tried to keep the changes so that, as a second pass, I think we could look at retaining the existing approach in places where no annotations occur. This might save memory but could cause strange behavior if AST transforms try to add type annotations to nodes. We'd probably need to enforce our pseudo immutable types if we do that second pass.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org