Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split ctorAttrs from attrs in codegen model StructDecl #4449

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

akosthekiss
Copy link
Contributor

This helps avoid dead code in ctorAttr initialization in some script targets (namely Python3 and JavaScript).

This helps avoid dead code in ctorAttr initialization in some
script targets (namely Python3 and JavaScript).

Signed-off-by: Akos Kiss <[email protected]>
@akosthekiss
Copy link
Contributor Author

I've created an example grammar:

grammar Attrs;

start
  returns [int r=0]
  locals [int l=1]
  : attr_a=a[1] attr_bc=bc[2, 3]
  ;

a[int x]
  returns [s:int=2]
  locals [m:int=3]
  : attr_A=A;

bc[y:int, z:int]
  returns [int t]
  locals [int n]
  : attr_B=B # b
  | attr_C=C # c
  ;

A: 'a';
B: 'b';
C: 'c';

And created a simple diff utility to check what difference the commit makes on all the supported targets:

ANTLR_DEV_DIR=~/devel/antlr4
ANTLR_JAR=~/.m2/repository/org/antlr/antlr4/4.13.2-SNAPSHOT/antlr4-4.13.2-SNAPSHOT-complete.jar
ATTRS_G4_DIR=$(pwd)

function generate { # $1:branch $2:dir
  cd $ANTLR_DEV_DIR
  git checkout $1
  mvn install -DskipTests=true

  cd $ATTRS_G4_DIR
  rm -rf $2
  for TARGET_LANG in Cpp CSharp Dart Go Java JavaScript PHP Python3 Swift TypeScript; do
    java -jar $ANTLR_JAR Attrs.g4 -no-listener -Dlanguage=$TARGET_LANG -o $2
  done
}

generate dev a
generate attr b
diff -r a b -C 5

The output is:

diff -r -C 5 a/AttrsParser.js b/AttrsParser.js
*** a/AttrsParser.js	Sat Oct 21 22:45:01 2023
--- b/AttrsParser.js	Sat Oct 21 22:45:14 2023
***************
*** 167,177 ****
              invokingState = -1;
          }
          super(parent, invokingState);
          this.parser = parser;
          this.ruleIndex = AttrsParser.RULE_a;
-         this.x = null
          this.s = 2
          this.m = 3
          this.attr_A = null;
          this.x = x || null;
      }
--- 167,176 ----
***************
*** 195,206 ****
              invokingState = -1;
          }
          super(parent, invokingState);
          this.parser = parser;
          this.ruleIndex = AttrsParser.RULE_bc;
-         this.y = null
-         this.z = null
          this.t = null
          this.n = null
          this.y = y || null;
          this.z = z || null;
      }
--- 194,203 ----
diff -r -C 5 a/AttrsParser.py b/AttrsParser.py
*** a/AttrsParser.py	Sat Oct 21 22:45:02 2023
--- b/AttrsParser.py	Sat Oct 21 22:45:15 2023
***************
*** 99,109 ****
          __slots__ = 'parser'
  
          def __init__(self, parser, parent:ParserRuleContext=None, invokingState:int=-1, x:int=None):
              super().__init__(parent, invokingState)
              self.parser = parser
-             self.x = None
              self.s = 2
              self.m = 3
              self.attr_A = None # Token
              self.x = x
  
--- 99,108 ----
***************
*** 137,148 ****
          __slots__ = 'parser'
  
          def __init__(self, parser, parent:ParserRuleContext=None, invokingState:int=-1, y:int=None, z:int=None):
              super().__init__(parent, invokingState)
              self.parser = parser
-             self.y = None
-             self.z = None
              self.t = None
              self.n = None
              self.y = y
              self.z = z
  
--- 136,145 ----

That is, the commit only makes the superfluous assignments go away in generated Python3 and JavaScript code. (I hope I managed to cover all the cases.)

@akosthekiss
Copy link
Contributor Author

Is this too big a change proposed for too little gain?

@ericvergnaud
Copy link
Contributor

@parrt this seems to make a lot of sense to me

@parrt
Copy link
Member

parrt commented Jul 28, 2024

oh man...i don't have to really think about this one. and as you say "big" change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants