You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by paulk-asert <gi...@git.apache.org> on 2018/09/08 00:23:47 UTC
[GitHub] groovy pull request #792: GROOVY-8778: Cast short-hand breaks for empty map
GitHub user paulk-asert opened a pull request:
https://github.com/apache/groovy/pull/792
GROOVY-8778: Cast short-hand breaks for empty map
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/paulk-asert/groovy groovy8778
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/groovy/pull/792.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #792
----
commit 921fbda4568592691fcda60919a90f93caf39135
Author: Paul King <pa...@...>
Date: 2018-09-08T00:22:37Z
GROOVY-8778: Cast short-hand breaks for empty map
----
---
[GitHub] groovy pull request #792: GROOVY-8778: Cast short-hand breaks for empty map
Posted by paulk-asert <gi...@git.apache.org>.
Github user paulk-asert commented on a diff in the pull request:
https://github.com/apache/groovy/pull/792#discussion_r216118264
--- Diff: subprojects/parser-antlr4/src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java ---
@@ -2337,14 +2337,11 @@ public Expression visitPathElement(PathElementContext ctx) {
this.visitNamedPropertyArgs(ctx.namedPropertyArgs());
Expression right;
- if (mapEntryExpressionList.size() == 1) {
- MapEntryExpression mapEntryExpression = mapEntryExpressionList.get(0);
-
- if (mapEntryExpression.getKeyExpression() instanceof SpreadMapExpression) {
- right = mapEntryExpression.getKeyExpression();
- } else {
- right = mapEntryExpression;
- }
+ if (mapEntryExpressionList.size() == 0) {
+ // expecting list of MapEntryExpressions later so use SpreadMap to smuggle empty MapExpression to later stages
+ right = new SpreadMapExpression(configureAST(new MapExpression(), ctx.namedPropertyArgs()));
--- End diff --
Yes, you are right - if we ever do support getAt(Map) more generally, we would need to process that differently, so best it is correct to start with.
---
[GitHub] groovy pull request #792: GROOVY-8778: Cast short-hand breaks for empty map
Posted by paulk-asert <gi...@git.apache.org>.
Github user paulk-asert commented on a diff in the pull request:
https://github.com/apache/groovy/pull/792#discussion_r216115626
--- Diff: subprojects/parser-antlr4/src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java ---
@@ -2337,14 +2337,11 @@ public Expression visitPathElement(PathElementContext ctx) {
this.visitNamedPropertyArgs(ctx.namedPropertyArgs());
Expression right;
- if (mapEntryExpressionList.size() == 1) {
- MapEntryExpression mapEntryExpression = mapEntryExpressionList.get(0);
-
- if (mapEntryExpression.getKeyExpression() instanceof SpreadMapExpression) {
- right = mapEntryExpression.getKeyExpression();
- } else {
- right = mapEntryExpression;
- }
+ if (mapEntryExpressionList.size() == 0) {
+ // expecting list of MapEntryExpressions later so use SpreadMap to smuggle empty MapExpression to later stages
+ right = new SpreadMapExpression(configureAST(new MapExpression(), ctx.namedPropertyArgs()));
--- End diff --
I had that originally but we throw that expression away in resolve visitor.
---
[GitHub] groovy pull request #792: GROOVY-8778: Cast short-hand breaks for empty map
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/groovy/pull/792
---
[GitHub] groovy pull request #792: GROOVY-8778: Cast short-hand breaks for empty map
Posted by danielsun1106 <gi...@git.apache.org>.
Github user danielsun1106 commented on a diff in the pull request:
https://github.com/apache/groovy/pull/792#discussion_r216113948
--- Diff: subprojects/parser-antlr4/src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java ---
@@ -2337,14 +2337,11 @@ public Expression visitPathElement(PathElementContext ctx) {
this.visitNamedPropertyArgs(ctx.namedPropertyArgs());
Expression right;
- if (mapEntryExpressionList.size() == 1) {
- MapEntryExpression mapEntryExpression = mapEntryExpressionList.get(0);
-
- if (mapEntryExpression.getKeyExpression() instanceof SpreadMapExpression) {
- right = mapEntryExpression.getKeyExpression();
- } else {
- right = mapEntryExpression;
- }
+ if (mapEntryExpressionList.size() == 0) {
+ // expecting list of MapEntryExpressions later so use SpreadMap to smuggle empty MapExpression to later stages
+ right = new SpreadMapExpression(configureAST(new MapExpression(), ctx.namedPropertyArgs()));
--- End diff --
It's better to `configureAST` the instance of `SpreadMapExpression` too, or its node position will be missing.
---
[GitHub] groovy pull request #792: GROOVY-8778: Cast short-hand breaks for empty map
Posted by danielsun1106 <gi...@git.apache.org>.
Github user danielsun1106 commented on a diff in the pull request:
https://github.com/apache/groovy/pull/792#discussion_r216116090
--- Diff: subprojects/parser-antlr4/src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java ---
@@ -2337,14 +2337,11 @@ public Expression visitPathElement(PathElementContext ctx) {
this.visitNamedPropertyArgs(ctx.namedPropertyArgs());
Expression right;
- if (mapEntryExpressionList.size() == 1) {
- MapEntryExpression mapEntryExpression = mapEntryExpressionList.get(0);
-
- if (mapEntryExpression.getKeyExpression() instanceof SpreadMapExpression) {
- right = mapEntryExpression.getKeyExpression();
- } else {
- right = mapEntryExpression;
- }
+ if (mapEntryExpressionList.size() == 0) {
+ // expecting list of MapEntryExpressions later so use SpreadMap to smuggle empty MapExpression to later stages
+ right = new SpreadMapExpression(configureAST(new MapExpression(), ctx.namedPropertyArgs()));
--- End diff --
The implementation of parser should not decouple with back end(i.e. not rely on the implementation of back end).
If the implementation of resolve visitor changes in the future, the expression is not thrown away, the node position is missing.
---