DFAState not set in ParserATNSimulator.AddDFAState()
kaby76 opened this issue · 3 comments
Just trying to eliminate the various diffs I am seeing in parser trace between Antlr4ng, CSharp, Java, JavaScript, in order to see why codeql/examples/alias.qll does not parse for Antlr4ng but does for all other targets.
The DFAState.statenNumber field is not being set in Antlr4ng here, but it is in all the other targets (CSharp, Java, JavaScript).
Oh, that's a difficult venture. The code more and more diverts. The state number is assigned in the new DFA.addState method. A class should manage its structures on its own and not be manipulated from outside. I mean, that's the purpose of classes, right? Otherwise we could just use records. The Java code is in a few places a bit sloppy, e.g. having public fields and accessors for these fields at the same time etc. I strove to make private what should be private, which later may allow to optimize internals without affecting the API of a class.
What I don't like about all this code in addDFAState() is there that "let's set the stateNumber" just before checking "hey, is this a read-only state??". Why are we setting the state number if it's read-only?? Kind of doesn't make sense. Agreed, the Java code is sloppy, but because I heard that "it's the gold standard", all the other code follows along happily--including all this sloppy stuff.
It's the configs that are set to readonly, not the DFA state itself, so for me this sounds sane.
About the "golden standard": I think we have seen enough runtime code that goes different ways, compared to the Java runtime. In the beginning a new runtime should probably follow the original approach, but at some point, if you want to take advantage of your target language, it's unavoidable to take different routes.