Java Backend: Add line number support. #217

guardbotmk3 · 2017-12-11T22:33:51Z

Use complex symbol for automatic context with syntax errors.

guardbotmk3 · 2017-12-11T22:34:52Z

Consider this a first pass. If there's something that doesn't match the project's style, or you'd like to see re-worked, let me know.

andreasabel · 2017-12-12T08:55:11Z

Thanks for the PR and your contribution to BNFC.

I took your PR as opportunity to put out some contribution guidelines, see README at https://github.com/BNFC/bnfc, section Contribute.

I am a bit confused about the title Java Backend: Add line number support. Looking at the existing code, there seems already to be line number support for the java backend. Maybe you meant column number?

Please add tests to the https://github.com/BNFC/bnfc/tree/master/testing infrastructure that document the new behavior.
Please document your changes in the code and the changelog.
From the style guide: Document function arguments, avoid Bool.

Looking forward to the updated PR!

andreasabel

Mostly request for documentation. Avoid magic numbers.

andreasabel · 2017-12-12T08:55:59Z

source/src/BNFC/Backend/Java.hs

-parserLexerSelector :: String -> JavaLexerParser -> ParserLexerSpecification
-parserLexerSelector _ JLexCup = ParseLexSpec
-    { lexer     = cf2JLex
+parserLexerSelector :: String -> JavaLexerParser -> Bool -> ParserLexerSpecification


Avoid Bool (or at least document function argument).

andreasabel · 2017-12-12T08:57:16Z

source/src/BNFC/Backend/Java/CFtoJLex15.hs

+        then if jflex then ""
+                      else "int yycolumn = -2;"
+        else vcat
+          [ "int yyline = -2;"


Magic numbers like -2 should be avoided.
If unavoidable, document properly. (Why -2, why doesn't it work without the magic?)

andreasabel · 2017-12-12T08:57:48Z

source/src/BNFC/Backend/Java/CFtoJLex15.hs

    , "%public"
    , "%{"
    , nest 2 $ vcat
        [ "String pstring = new String();"
+        , "ComplexSymbolFactory.Location left = new ComplexSymbolFactory.Location(-1, -1);"


Document magic numbers (-1).

andreasabel · 2017-12-12T08:58:31Z

source/src/BNFC/Backend/Java/CFtoJLex15.hs

+            then "right.move(0, yylength(), yylength());"
+            else "")
+        , "  return right;"
+        , "}"
        , "public String buff()" <+> braces
            (if jflex
            then "return new String(zzBuffer,zzCurrentPos,10).trim();"


Magic number 10.

Magic number 10 was there before.

guardbotmk3 · 2017-12-12T21:28:49Z

I am a bit confused about the title Java Backend: Add line number support. Looking at the existing code, there seems already to be line number support for the java backend. Maybe you meant column number?

No. The line number support currently extends only as far the lexer. The symbols created for the parser have no line information, so syntax errors have no context info to find them.

ComplexSymbol is necessary because the standard JLex Symbol class only keeps track of character position, oddly enough.

Expect an eventual followup to record the line info in the generated abstract syntax tree.

guardbotmk3 · 2017-12-13T01:56:27Z

Added tests to the testsuite, and verified they pass for jlex and jflex, both with and without line numbers.

andreasabel · 2017-12-13T08:20:50Z

Thanks for the quick response!

Will you update this PR or make a new one, with the changes you outlined above?

guardbotmk3 · 2017-12-13T16:09:57Z

It's already updated. Edit: I realized you may have meant adding line info the Abstract Syntax Tree. I'll do that in a separate PR, but the other changes are already in this one.

…

On Dec 13, 2017 12:20 AM, "Andreas Abel" ***@***.***> wrote: Thanks for the quick response! Will you update this PR or make a new one, with the changes you outlined above? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#217 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AgRziCtUftUy-R9wtQ5fK-4MBIk0MAeJks5s_4jjgaJpZM4Q-IuU> .

andreasabel · 2017-12-13T21:27:16Z

Thanks for the update. Almost there, can we make the title a bit more precise?
"Java Backend: Add line number support." suggests that there was no line number support even though the help text says

Special options for the Java backend
  -l                        Add and set line_number field for all syntax classes

The new text should make clear

what did not work about the existing -l option
what works now
what does not work yet.

Please write this up in a paragraph or so and put it into Java.hs, and then update the title for more precision.

Thanks for your contribution so far!

Currently the help message includes the following for Java: Special options for the Java backend -l Add and set line_number field for all syntax classes The only place this support existed was in the help documentation. A quick glance through the Java backend files will show that the Option.linenumbers was never inspected. This change adds that support for parsing. Cup's "Symbol" class only supports character offsets, so we use its "ComplexSymbol" class, which supports line, column, and offset. JLex does not provide column info, but JFlex does. ComplexSymbol tracks the left and right extents of a production, and so for variable length tokens we capture this info. The abstract syntax classes still do not record line information, but the necessary information is now available during parsing, and a follow-up commit will handle adding the info to the Absyn classes and code to record it to the generated cup file.

guardbotmk3 · 2017-12-18T18:37:33Z

I added the additional documentation to the commit message. Java.hs didn't seem like the right place for it.
The short answer as to what didn't work is "any of it". If you look at the existing code, nothing in the Java Backend even checks Options.linenumbers. The commit also clarifies what is working post commit, and what still needs to be done.

andreasabel · 2017-12-18T20:41:58Z

Thanks for the good work!

andreasabel · 2017-12-18T21:07:24Z

Er, the testsuite does not pass. I am getting errors like:

C4/Yylex.java:299: error: incompatible types: int cannot be converted to String
    return new ComplexSymbolFactory.Location(yyline+1, yycolumn+1, yychar);
                                                   ^
C4/Yylex.java:303: error: cannot find symbol
  right.move(0, yylength(), yylength());
       ^

According to http://czt.sourceforge.net/dev/java-cup-runtime/apidocs/java_cup/runtime/ComplexSymbolFactory.Location.html there is no Location constructor of the type you use.

Could be that your patch requires a different version of cup?

ubuntu xenial comes with version 0.11a.

apt list -v cup
cup/xenial,xenial,now 0.11a+20060608-6 all [installed]
  LALR parser generator for Java(tm)

What is the minimal version of cup you require?

andreasabel · 2017-12-18T21:31:29Z

It seems that with the latest version of java-cup, there are no problems with the testsuite.

How can we ensure that or test whether the necessary version is installed?

guardbotmk3 · 2017-12-18T23:17:12Z

With the move in there, it requires a fairly new cup, 2015 or so. Without it, any cup 0.11b will do.

I can also adjust the code to work with 0.11a. I have that working already. I'll create a new PR that does that.

andreasabel requested changes Dec 12, 2017

View reviewed changes

guardbotmk3 force-pushed the add-line-numbering-java branch from d88b429 to 9bae6f9 Compare December 12, 2017 21:27

andreasabel self-assigned this Dec 13, 2017

andreasabel added the Java label Dec 13, 2017

andreasabel added this to the 2.8.2 milestone Dec 13, 2017

andreasabel approved these changes Dec 13, 2017

View reviewed changes

guardbotmk3 force-pushed the add-line-numbering-java branch from 9bae6f9 to 856d2db Compare December 18, 2017 18:34

andreasabel merged commit 0e92612 into BNFC:master Dec 18, 2017

andreasabel added a commit that referenced this pull request Jan 1, 2018

[ changelog ] for #217

cac589c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Java Backend: Add line number support. #217

Java Backend: Add line number support. #217

Java Backend: Add line number support. #217

Java Backend: Add line number support. #217

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment