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

Port PredictionMode #134

Merged
merged 1 commit into from
Oct 19, 2016
Merged

Port PredictionMode #134

merged 1 commit into from
Oct 19, 2016

Conversation

sharwell
Copy link
Member

No description provided.


export namespace PredictionMode {
/** A Map that uses just the state and the stack context as the key. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a note, I was surprised by this. Given the name "PredictionMode", I expected this to be a simple enum.

Copy link
Member Author

Choose a reason for hiding this comment

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

💭 I think a lot of the functionality in PredictionMode could actually be removed from the optimized fork. We'll see at the end.

Copy link
Member Author

Choose a reason for hiding this comment

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

➡️ Filed #148

@@ -223,7 +241,7 @@ export enum PredictionMode {
* the configurations to strip out all of the predicates so that a standard
* {@link ATNConfigSet} will merge everything ignoring predicates.</p>
*/
static hasSLLConflictTerminatingPrediction(mode: PredictionMode, @NotNull configs: ATNConfigSet): boolean {
export function hasSLLConflictTerminatingPrediction(mode: PredictionMode, /*@NotNull*/ configs: ATNConfigSet): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an example of a place where @NotNull seems to have been commented out unnecessarily. That's not to say I'm arguing for keeping them, I think it reads more clearly if completely eliminated.

Copy link
Member Author

Choose a reason for hiding this comment

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

➡️ I've used the comments in a few cases when porting other files. I want to leave them in at least until we're done getting things working. In this case, the function is not a member of a class so decorators are not allowed. Commenting and removing were the only options allowed by the language.

@BurtHarris BurtHarris merged commit cd3b54c into master Oct 19, 2016
@BurtHarris BurtHarris deleted the prediction-mode branch October 19, 2016 18:44
@BurtHarris
Copy link
Collaborator

OK by me.. Should I remove both code review and comment review flags?

@sharwell
Copy link
Member Author

@BurtHarris The needs comment review is a special label for me. I sent you a Gitter message about it.

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

Successfully merging this pull request may close these issues.

None yet

2 participants