Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

fixed issue #170, hopefully #176

Closed
wants to merge 2 commits into from
Closed

fixed issue #170, hopefully #176

wants to merge 2 commits into from

Conversation

hgdsraj
Copy link
Contributor

@hgdsraj hgdsraj commented Nov 10, 2016

Fixed issue #170
#170

Added virtual keyword to storage.type.modfier's list.

Copy link
Contributor

@winstliu winstliu left a comment

Choose a reason for hiding this comment

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

This will need some specs before it can be merged.

@@ -277,7 +277,7 @@
]
}
{
'begin': '\\b(class|struct)\\b\\s*([_A-Za-z][_A-Za-z0-9]*\\b)?+(\\s*:\\s*(public|protected|private)\\s*([_A-Za-z][_A-Za-z0-9]*\\b)((\\s*,\\s*(public|protected|private)\\s*[_A-Za-z][_A-Za-z0-9]*\\b)*))?'
'begin': '\\b(class|struct)\\b\\s*([_A-Za-z][_A-Za-z0-9]*\\b)?+(\\s*:\\s*(public|protected|private|virtual)\\s*([_A-Za-z][_A-Za-z0-9]*\\b)((\\s*,\\s*(public|protected|private)\\s*[_A-Za-z][_A-Za-z0-9]*\\b)*))?'
Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot to add virtual to the second time the public|protected|private list occurs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added :)

@hgdsraj
Copy link
Contributor Author

hgdsraj commented Nov 25, 2016

What changes are necessary?

@winstliu
Copy link
Contributor

As I said in the review, this will need specs before it can be merged.

@thomasjo
Copy link
Contributor

This change doesn't make sense. The original assignment of the virtual keyword was correct.
In other words, this is not the correct way to fix #170. Seems like that issue will need a deeper investigation, and it will likely require a slightly more tricky grammar change.

Such an effort should be started with a failing/red spec that tests the correct, expected behavior. Once the grammar bug has been resolved, that spec should become green.

Since it is likely less effort to start a fresh PR instead of rebasing this, and then starting over, I'm simply going to close this. If you want to help us out by fixing #170 properly, please open a new PR. Feel free to open it early with the failing spec, and I'm sure someone will be able to help you figure out how to properly resolve this bug. Thanks! 🙇

@thomasjo thomasjo closed this Nov 30, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants