-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Update vba.g4 #3880
base: master
Are you sure you want to change the base?
Update vba.g4 #3880
Conversation
bug fix for line labels, allowing line labels to have white space after the colon
I just submitted a VBA pull request as well. #3881 Let me know if you have any advice on what to do or not to do regarding ANTLR and Linting VBA. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add/extend an example that covers suggested changes to the examples subdirecotry.
The sub directory should already have line numbers i added last patch of mine, this one just fixes them since before if your line numbers had white space after them they didnt parse correctly. |
@Xenios91 would it be worthwhile adding some whitespace to one of the line numbered lines, to demonstrate that that case fails under the existing grammar, but passes under the new grammar? |
@KvanTTT good to merge this? |
No, I suggest adding a test data in the way @Beakerboy suggested. |
@Xenios91 The VBA editor that ships with excel automatically strips trailing whitespace out of a line label. If the line label is on the same line as a statement, there can be whitespace between them. Should the whitespace be considered part of the label, or as a separator between different contexts? Using underscore for trailing whitespace:
|
Right, I have just come across documents in the wild that it fails to parse them correctly when there is this white space, so while it should be removed, it seems like it is not. I speculate this may be due to a document being generated by something like apache poi or another library where the vba formatter doesnt clean it up. Id say for now maybe just an optional character for line labels, as making it a separator may impact other parts of the grammar. The more I think about it, i believe a lot of the parsing errors I am seeing are from automated techniques to generate documents where that VBA cleanup doesnt occur. |
I had not heard of Apache POI, so I looked it up. I’ve been working on my own OLE file writer for quite a while. I wish I knew this was out there. Thanks! |
oh yeah its pretty nice! |
test added |
@@ -9,4 +9,5 @@ Dim Number, MyString | |||
MyString = "Number equals 2" | |||
3: | |||
Debug.Print MyString | |||
4: Debug.Print "TEST" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't test the changed grammar rule "lineLabel". There's not a single test that has "lineLabel".
01/26-05:00:52 ~/issues/g4-3880/vba/Generated-CSharp
$ trparse ../examples/*.bas | trxgrep ' //lineLabel' | trtext -c
CSharp 0 ../examples/example09_typeHints.bas success 0.3759407
CSharp 0 ../examples/example1.bas success 0.0414117
CSharp 0 ../examples/example2operators.bas success 0.1742185
CSharp 0 ../examples/example3erase.bas success 0.0385953
CSharp 0 ../examples/example4fixedstring.bas success 0.006915
CSharp 0 ../examples/example5property.bas success 0.0179688
CSharp 0 ../examples/example6dateliteral.bas success 0.0156073
CSharp 0 ../examples/example7linecontinuation.bas success 0.3410573
CSharp 0 ../examples/example8_lineNumbers.bas success 0.3330462
../examples/example09_typeHints.bas:0
../examples/example1.bas:0
../examples/example2operators.bas:0
../examples/example3erase.bas:0
../examples/example4fixedstring.bas:0
../examples/example5property.bas:0
../examples/example6dateliteral.bas:0
../examples/example7linecontinuation.bas:0
../examples/example8_lineNumbers.bas:0
01/26-05:03:05 ~/issues/g4-3880/vba/Generated-CSharp
$
Here's a "coverage heat map" for the tests (rename to cover.html and view in an internet browser).
cover.html.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if there was a way to see coverage. Unfortunately, removing the ‘.txt’ just gives a 404 error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Download cover.html.txt (I just tried the link, it works). Save it on disk. Rename cover.html.txt to cover.html in a command-line shell. Open a browser and click to open cover.html.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kaby76 Do you know if there is a way to see the coverage file from a working branch on GitHub? I’m adding examples and want to see which tokens I’m missing along the way.
I’m used to sending coverage reports to coveralls.io.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverage is not run in the Github Actions build because it is time consuming. #3624
You'll have to run it locally on your machine.
does the block section need to be changed to
and then remove lineLabel from blockStmt. I feel this should ensure that the line label can only occur without leading whitespace, and also that is properly identified when it is alone on a line and when it is followed with statements. can line numbers also appear on the lame line as other statements. The grammar currently states that there must be a line break at the end. Either way, lineNumber would probably be handled similarly. |
One issue I see…how is the parser supposed to know that when it sees:
that it is a line labeled ‘foo’ versus a call to the foo subroutine, followed by a let statement? |
bug fix for line labels, allowing line labels to have white space after the colon