[GitHub] groovy pull request #472: Macro methods

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/groovy/pull/472


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #472: Macro methods

[bsideup]

Github user bsideup commented on a diff in the pull request:

https://github.com/apache/groovy/pull/472#discussion_r108093778
  
--- Diff: 
subprojects/groovy-macro/src/main/groovy/org/codehaus/groovy/macro/transform/MacroMethodsCache.java
 ---
@@ -0,0 +1,144 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ */
+package org.codehaus.groovy.macro.transform;
+
+import org.codehaus.groovy.ast.ClassHelper;
+import org.codehaus.groovy.ast.ClassNode;
+import org.codehaus.groovy.ast.MethodNode;
+import org.codehaus.groovy.ast.Parameter;
+import org.codehaus.groovy.macro.runtime.Macro;
+import org.codehaus.groovy.runtime.m12n.ExtensionModule;
+import org.codehaus.groovy.runtime.m12n.ExtensionModuleScanner;
+import org.codehaus.groovy.runtime.m12n.MetaInfExtensionModule;
+import org.codehaus.groovy.transform.stc.ExtensionMethodNode;
+
+import java.util.*;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+/**
+ * TODO share some code with {@link 
org.codehaus.groovy.transform.stc.StaticTypeCheckingSupport.ExtensionMethodCache}
+ * @author Sergei Egorov 
+ * @since 2.5.0
+ */
+class MacroMethodsCache {
--- End diff --

We could once parrot is merged to master :) 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #472: Macro methods

[danielsun1106]

Github user danielsun1106 commented on a diff in the pull request:

https://github.com/apache/groovy/pull/472#discussion_r108077305
  
--- Diff: 
subprojects/groovy-macro/src/main/groovy/org/codehaus/groovy/macro/transform/MacroMethodsCache.java
 ---
@@ -0,0 +1,144 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ */
+package org.codehaus.groovy.macro.transform;
+
+import org.codehaus.groovy.ast.ClassHelper;
+import org.codehaus.groovy.ast.ClassNode;
+import org.codehaus.groovy.ast.MethodNode;
+import org.codehaus.groovy.ast.Parameter;
+import org.codehaus.groovy.macro.runtime.Macro;
+import org.codehaus.groovy.runtime.m12n.ExtensionModule;
+import org.codehaus.groovy.runtime.m12n.ExtensionModuleScanner;
+import org.codehaus.groovy.runtime.m12n.MetaInfExtensionModule;
+import org.codehaus.groovy.transform.stc.ExtensionMethodNode;
+
+import java.util.*;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+/**
+ * TODO share some code with {@link 
org.codehaus.groovy.transform.stc.StaticTypeCheckingSupport.ExtensionMethodCache}
+ * @author Sergei Egorov 
+ * @since 2.5.0
+ */
+class MacroMethodsCache {
--- End diff --

Maybe you can reuse LRU cache in the parrot branch: 
https://issues.apache.org/jira/browse/GROOVY-7977


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #472: Macro methods

[melix]

Github user melix commented on a diff in the pull request:

https://github.com/apache/groovy/pull/472#discussion_r103271106
  
--- Diff: 
subprojects/groovy-macro/src/main/groovy/org/codehaus/groovy/macro/transform/MacroTransformation.java
 ---
@@ -18,42 +18,113 @@
  */
 package org.codehaus.groovy.macro.transform;
 
+import groovy.transform.CompilationUnitAware;
 import org.codehaus.groovy.ast.*;
 import org.codehaus.groovy.ast.expr.*;
+import org.codehaus.groovy.classgen.asm.InvocationWriter;
+import org.codehaus.groovy.control.CompilationUnit;
 import org.codehaus.groovy.control.CompilePhase;
 import org.codehaus.groovy.control.SourceUnit;
+import org.codehaus.groovy.macro.runtime.MacroContext;
+import org.codehaus.groovy.macro.runtime.MacroStub;
+import org.codehaus.groovy.runtime.InvokerHelper;
 import org.codehaus.groovy.transform.GroovyASTTransformation;
+import org.codehaus.groovy.transform.stc.ExtensionMethodNode;
+import org.codehaus.groovy.transform.stc.StaticTypeCheckingSupport;
+
+import java.util.ArrayList;
+import java.util.List;
 
 /**
- *
  * @author Sergei Egorov 
  */
 
 @GroovyASTTransformation(phase = CompilePhase.CONVERSION)
-public class MacroTransformation extends MethodCallTransformation {
+public class MacroTransformation extends MethodCallTransformation 
implements CompilationUnitAware {
+
+private static final ClassNode MACRO_CONTEXT_CLASS_NODE = 
ClassHelper.make(MacroContext.class);
+
+private static final ClassNode MACRO_STUB_CLASS_NODE = 
ClassHelper.make(MacroStub.class);
+
+private static final PropertyExpression MACRO_STUB_INSTANCE = new 
PropertyExpression(new ClassExpression(MACRO_STUB_CLASS_NODE), "INSTANCE");
+
+private static final String MACRO_STUB_METHOD_NAME = "macroMethod";
+
+protected CompilationUnit unit;
 
-public static final String DOLLAR_VALUE = "$v";
-public static final String MACRO_METHOD = "macro";
+@Override
+public void setCompilationUnit(CompilationUnit unit) {
+this.unit = unit;
+}
 
 @Override
-protected GroovyCodeVisitor getTransformer(final ASTNode[] nodes, 
final SourceUnit sourceUnit) {
-final ClassCodeExpressionTransformer trn = new 
ClassCodeExpressionTransformer() {
+protected GroovyCodeVisitor getTransformer(ASTNode[] nodes, final 
SourceUnit sourceUnit) {
+// Macro methods should on a classpath of the compiler because we 
invoke them during the compilation
+final ClassLoader classLoader = this.getClass().getClassLoader();
+return new ClassCodeVisitorSupport() {
+
 @Override
 protected SourceUnit getSourceUnit() {
 return sourceUnit;
 }
 
 @Override
-public Expression transform(final Expression exp) {
-if (exp instanceof ConstructorCallExpression) {
-MethodCallExpression call = 
exp.getNodeMetaData(MacroTransformation.class);
-if (call!=null) {
-return call;
+public void visitMethodCallExpression(MethodCallExpression 
call) {
--- End diff --

This method is very long. Could you split it into smaller focused pieces?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #472: Macro methods

[melix]

Github user melix commented on a diff in the pull request:

https://github.com/apache/groovy/pull/472#discussion_r103270568
  
--- Diff: 
subprojects/groovy-macro/src/main/groovy/org/codehaus/groovy/macro/transform/MacroClassTransformation.java
 ---
@@ -0,0 +1,114 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ */
+package org.codehaus.groovy.macro.transform;
+
+import org.codehaus.groovy.ast.*;
+import org.codehaus.groovy.ast.expr.ConstructorCallExpression;
+import org.codehaus.groovy.ast.expr.Expression;
+import org.codehaus.groovy.ast.expr.MethodCallExpression;
+import org.codehaus.groovy.ast.tools.GeneralUtils;
+import org.codehaus.groovy.control.CompilePhase;
+import org.codehaus.groovy.control.SourceUnit;
+import org.codehaus.groovy.macro.methods.MacroGroovyMethods;
+import org.codehaus.groovy.macro.runtime.MacroBuilder;
+import org.codehaus.groovy.transform.GroovyASTTransformation;
+
+import java.util.Iterator;
+import java.util.List;
+
+import static org.codehaus.groovy.ast.tools.GeneralUtils.*;
+
+@GroovyASTTransformation(phase = CompilePhase.CONVERSION)
+public class MacroClassTransformation extends MethodCallTransformation {
+
+private static final String MACRO_METHOD = "macro";
+private static final ClassNode MACROCLASS_TYPE = 
ClassHelper.make(MacroClass.class);
+
+@Override
+protected GroovyCodeVisitor getTransformer(final ASTNode[] nodes, 
final SourceUnit sourceUnit) {
+ClassCodeExpressionTransformer transformer = new 
ClassCodeExpressionTransformer() {
+@Override
+protected SourceUnit getSourceUnit() {
+return sourceUnit;
+}
+
+@Override
+public Expression transform(final Expression exp) {
+if (exp instanceof ConstructorCallExpression) {
+MethodCallExpression call = 
exp.getNodeMetaData(MacroTransformation.class);
+if (call != null) {
+return call;
+}
+}
+return super.transform(exp);
+}
+};
+
+return new TransformingCodeVisitor(transformer) {
--- End diff --

Same here. Could you extract a class for this?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #472: Macro methods

[melix]

Github user melix commented on a diff in the pull request:

https://github.com/apache/groovy/pull/472#discussion_r103270486
  
--- Diff: 
subprojects/groovy-macro/src/main/groovy/org/codehaus/groovy/macro/transform/MacroClassTransformation.java
 ---
@@ -0,0 +1,114 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ */
+package org.codehaus.groovy.macro.transform;
+
+import org.codehaus.groovy.ast.*;
+import org.codehaus.groovy.ast.expr.ConstructorCallExpression;
+import org.codehaus.groovy.ast.expr.Expression;
+import org.codehaus.groovy.ast.expr.MethodCallExpression;
+import org.codehaus.groovy.ast.tools.GeneralUtils;
+import org.codehaus.groovy.control.CompilePhase;
+import org.codehaus.groovy.control.SourceUnit;
+import org.codehaus.groovy.macro.methods.MacroGroovyMethods;
+import org.codehaus.groovy.macro.runtime.MacroBuilder;
+import org.codehaus.groovy.transform.GroovyASTTransformation;
+
+import java.util.Iterator;
+import java.util.List;
+
+import static org.codehaus.groovy.ast.tools.GeneralUtils.*;
+
+@GroovyASTTransformation(phase = CompilePhase.CONVERSION)
+public class MacroClassTransformation extends MethodCallTransformation {
+
+private static final String MACRO_METHOD = "macro";
+private static final ClassNode MACROCLASS_TYPE = 
ClassHelper.make(MacroClass.class);
+
+@Override
+protected GroovyCodeVisitor getTransformer(final ASTNode[] nodes, 
final SourceUnit sourceUnit) {
+ClassCodeExpressionTransformer transformer = new 
ClassCodeExpressionTransformer() {
--- End diff --

I think this would benefit from being a static inner class. There's no 
clear interest in keeping a reference back to the transformation itself.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #472: Macro methods

[melix]

Github user melix commented on a diff in the pull request:

https://github.com/apache/groovy/pull/472#discussion_r103271019
  
--- Diff: 
subprojects/groovy-macro/src/main/groovy/org/codehaus/groovy/macro/transform/MacroTransformation.java
 ---
@@ -18,42 +18,113 @@
  */
 package org.codehaus.groovy.macro.transform;
 
+import groovy.transform.CompilationUnitAware;
 import org.codehaus.groovy.ast.*;
 import org.codehaus.groovy.ast.expr.*;
+import org.codehaus.groovy.classgen.asm.InvocationWriter;
+import org.codehaus.groovy.control.CompilationUnit;
 import org.codehaus.groovy.control.CompilePhase;
 import org.codehaus.groovy.control.SourceUnit;
+import org.codehaus.groovy.macro.runtime.MacroContext;
+import org.codehaus.groovy.macro.runtime.MacroStub;
+import org.codehaus.groovy.runtime.InvokerHelper;
 import org.codehaus.groovy.transform.GroovyASTTransformation;
+import org.codehaus.groovy.transform.stc.ExtensionMethodNode;
+import org.codehaus.groovy.transform.stc.StaticTypeCheckingSupport;
+
+import java.util.ArrayList;
+import java.util.List;
 
 /**
- *
  * @author Sergei Egorov 
  */
 
 @GroovyASTTransformation(phase = CompilePhase.CONVERSION)
-public class MacroTransformation extends MethodCallTransformation {
+public class MacroTransformation extends MethodCallTransformation 
implements CompilationUnitAware {
+
+private static final ClassNode MACRO_CONTEXT_CLASS_NODE = 
ClassHelper.make(MacroContext.class);
+
+private static final ClassNode MACRO_STUB_CLASS_NODE = 
ClassHelper.make(MacroStub.class);
+
+private static final PropertyExpression MACRO_STUB_INSTANCE = new 
PropertyExpression(new ClassExpression(MACRO_STUB_CLASS_NODE), "INSTANCE");
+
+private static final String MACRO_STUB_METHOD_NAME = "macroMethod";
+
+protected CompilationUnit unit;
 
-public static final String DOLLAR_VALUE = "$v";
-public static final String MACRO_METHOD = "macro";
+@Override
+public void setCompilationUnit(CompilationUnit unit) {
+this.unit = unit;
+}
 
 @Override
-protected GroovyCodeVisitor getTransformer(final ASTNode[] nodes, 
final SourceUnit sourceUnit) {
-final ClassCodeExpressionTransformer trn = new 
ClassCodeExpressionTransformer() {
+protected GroovyCodeVisitor getTransformer(ASTNode[] nodes, final 
SourceUnit sourceUnit) {
+// Macro methods should on a classpath of the compiler because we 
invoke them during the compilation
+final ClassLoader classLoader = this.getClass().getClassLoader();
+return new ClassCodeVisitorSupport() {
+
 @Override
 protected SourceUnit getSourceUnit() {
 return sourceUnit;
 }
 
 @Override
-public Expression transform(final Expression exp) {
-if (exp instanceof ConstructorCallExpression) {
-MethodCallExpression call = 
exp.getNodeMetaData(MacroTransformation.class);
-if (call!=null) {
-return call;
+public void visitMethodCallExpression(MethodCallExpression 
call) {
+super.visitMethodCallExpression(call);
+
+List methods = 
MacroMethodsCache.get(classLoader).get(call.getMethodAsString());
+
+if (methods == null) {
+// Not a macro call
+return;
+}
+
+List callArguments = 
InvocationWriter.makeArgumentList(call.getArguments()).getExpressions();
+
+ClassNode[] argumentsList = new 
ClassNode[callArguments.size()];
+
+for (int i = 0; i < callArguments.size(); i++) {
+argumentsList[i] = 
ClassHelper.make(callArguments.get(i).getClass());
+}
+
+methods = 
StaticTypeCheckingSupport.chooseBestMethod(MACRO_CONTEXT_CLASS_NODE, methods, 
argumentsList);
+
+for (MethodNode macroMethodNode : methods) {
+if (!(macroMethodNode instanceof ExtensionMethodNode)) 
{
+// TODO is it even possible?
--- End diff --

If you don't know if it's possible, it's probably better to throw an error. 
Then if the case appears we know we need to fix it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #472: Macro methods

[melix]

Github user melix commented on a diff in the pull request:

https://github.com/apache/groovy/pull/472#discussion_r103269693
  
--- Diff: 
subprojects/groovy-macro/src/main/groovy/org/codehaus/groovy/macro/runtime/MacroStub.java
 ---
@@ -0,0 +1,32 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ */
+package org.codehaus.groovy.macro.runtime;
+
+/**
+ * Stub for macro calls.
--- End diff --

Would you mind giving more context on this? It might be hard for newcomers 
to figure out.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] groovy pull request #472: Macro methods

[melix]

Github user melix commented on a diff in the pull request:

https://github.com/apache/groovy/pull/472#discussion_r103269312
  
--- Diff: 
subprojects/groovy-macro/src/main/groovy/org/codehaus/groovy/macro/runtime/Macro.java
 ---
@@ -0,0 +1,36 @@
+/*
+ *  Licensed to the Apache Software Foundation (ASF) under one
+ *  or more contributor license agreements.  See the NOTICE file
+ *  distributed with this work for additional information
+ *  regarding copyright ownership.  The ASF licenses this file
+ *  to you under the Apache License, Version 2.0 (the
+ *  "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing,
+ *  software distributed under the License is distributed on an
+ *  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ *  KIND, either express or implied.  See the License for the
+ *  specific language governing permissions and limitations
+ *  under the License.
+ */
+package org.codehaus.groovy.macro.runtime;
+
+import org.apache.groovy.lang.annotation.Incubating;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+/**
+ * @author Sergei Egorov 
+ */
+
+@Retention(RetentionPolicy.RUNTIME)
+@Target({ElementType.METHOD})
+@Incubating
+public @interface Macro {
--- End diff --

Missing `@since`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---