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.

Sign up for GitHub

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

added some properties and methods #4525

Closed
wants to merge 4 commits into from
Closed

added some properties and methods #4525

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ee4c88f
added some properties and methods
RobEin Feb 4, 2024
9193b09
Merge branch 'antlr:dev' into dev
RobEin Mar 5, 2024
29a3d4e
added some properties and methods (2)
RobEin Mar 5, 2024
dbf7983
Merge branch 'antlr:dev' into dev
RobEin Mar 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions runtime/JavaScript/src/antlr4/Lexer.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,16 @@ export declare class Lexer extends Recognizer<number> {
_tokenStartLine: number;
_tokenStartColumn: number;
_type: number;
_modeStack: number[];
_mode: number;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe rather add getMode an getModeStack ?

Copy link
Contributor Author

@RobEin RobEin Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The public fields would be better for me because I have also used them in my various portings:
Java port
JavaScript port
Pyton port

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well these fields are clearly private... (I guess there was a bit of laziness with the first implementation 30 years ago)


constructor(input: CharStream);
reset(): void;
nextToken(): Token;
skip(): void;
more(): void;
more(m: number): void;
mode(m: number): void;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the intent with the 2nd more was to be mode. Can you merge these ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 2 more are already in the current Lexer.d.ts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should only be 1, it's a bug in the existing file.

pushMode(m: number): void;
popMode(): number;
emitToken(token: Token): void;
Expand Down
2 changes: 2 additions & 0 deletions runtime/JavaScript/src/antlr4/Recognizer.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,6 @@ export declare class Recognizer<TSymbol> {

removeErrorListeners(): void;
addErrorListener(listener: ErrorListener<TSymbol>): void;
getSymbolicNames(): string[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also add getLiteralNames ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add all JavaScript attributes, but you wrote: "I only export what is strictly required"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe in encapsulation. I'm ok to provide data via methods, not so much direct read/write access to fields that may change anytime... Adding getLiteralNames is perfectly fine.

getErrorListenerDispatch(): ErrorListener<TSymbol>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename this to say getRootErrorListener() (open to suggestions). Appreciate it's an instance of ErrorListenerDispatch but that's a terrible name...

Copy link
Contributor Author

@RobEin RobEin Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name mode is really not a good property name, because there is already a mode() method in the JavaScript Lexer.
Maybe a property for JavaScript/TypeScript called lexerMode or tokenizerMode?
And the mode() method would be depreciated in the JavaScript Lexer.

I don't like the name getErrorListenerDispatch either.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we can't change that name without breaking backwards compatibility in JS...

Copy link
Contributor Author

@RobEin RobEin Mar 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, unfortunately the getErrorListenerDispatch method should not be renamed.
I think an independent TypeScript runtime (no *.js, no *.d.ts) would be ideal for this and other more modern designs.
Maybe in ANTLR5?

I remove my setMode method because there is already a mode method that does the same thing.
However, a setMode/getMode pair would have been more elegant than a mode/getMode pair.

}
3 changes: 3 additions & 0 deletions runtime/JavaScript/src/antlr4/Token.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import {CharStream} from "./CharStream";
import {TokenSource} from "./TokenSource";
import {InputStream} from "./InputStream";

export declare class Token {

Expand All @@ -7,6 +9,7 @@ export declare class Token {
static DEFAULT_CHANNEL: number;
static HIDDEN_CHANNEL: number;

source: [ TokenSource, InputStream ];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather you add getTokenSource and getInputStream. source being a tuple is an internal detail

Copy link
Contributor Author

@RobEin RobEin Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a method where I refer to the source property and a getTokenSource() would look strange in my opinion:.

private getCommonTokenByToken(oldToken: Token): CommonToken {
    const cToken = new CommonToken(oldToken.source, oldToken.type, oldToken.channel, oldToken.start, oldToken.stop);
    cToken.tokenIndex = oldToken.tokenIndex;
    cToken.line = oldToken.line;
    cToken.column = oldToken.column;
    cToken.text = oldToken.text;
    return cToken;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, maybe what I wrote was a little misunderstood.
Perhaps it would have been more understandable if I had referred to a public field instead of a property.
However, VS Code marks public fields as properties.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really not keen to expose such a poorly designed construct. The d.ts file already has getInputStream, so you only need to add getTokenSource.

tokenIndex: number;
line: number;
column: number;
Expand Down
Loading