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

Absyn line tracking #224

Merged
merged 3 commits into from
Jan 18, 2018
Merged

Conversation

guardbotmk3
Copy link

Adds support for line, column, offset tracking in the Abstract Syntax classes.
Because the location support in cup 0.11a is spotty, 0.11a is almost 12 years old, and location support didn't work until recently, I decided that for location support, we would require cup 0.11b-2014-06-11 or later. I verified that without the -l option, cup 0.11a can still process the resulting files.
2552ae9 contains some edits besides the reversion that allow it to work with 0.11a when line number support is off.

We'll have to work something out with travis to test the -l option only with a newish cup.

@guardbotmk3
Copy link
Author

Ping?

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Style issue: do not use Bool.

@@ -352,7 +353,7 @@ cf2AntlrLex' l = CF2Lex

-- | CF -> PARSER GENERATION TOOL BRIDGE
-- | function translating the CF to an appropriate parser generation tool.
type CF2ParserFunction = String -> String -> CF -> SymEnv -> String
type CF2ParserFunction = String -> String -> CF -> Bool -> SymEnv -> String
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use plain Bool. Define a 2-value data type with telling names instead. Or at least define a type synonym.

@@ -53,8 +53,9 @@ type MetaVar = (String, Cat)

-- | Creates the ANTLR parser grammar for this CF.
--The environment comes from CFtoAntlr4Lexer
cf2AntlrParse :: String -> String -> CF -> SymEnv -> String
cf2AntlrParse packageBase packageAbsyn cf env = unlines
--The bool is line numbering, which is currently not supported.
Copy link
Member

Choose a reason for hiding this comment

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

Put argument documentations down as haddock comments, i.e, a -- ^ comment following the argument type. See https://github.com/andreasabel/haskell-style-guide/blob/master/haskell-style.md

@andreasabel
Copy link
Member

Thanks for the contribution!
One of my missions is to improve the code quality of bnfc, in terms of documentation, maintainability, automatic testing, etc. Here is a style guide for Haskell code (WIP): https://github.com/andreasabel/haskell-style-guide/blob/master/haskell-style.md

@andreasabel andreasabel self-assigned this Jan 17, 2018
@guardbotmk3 guardbotmk3 force-pushed the absyn-line-tracking branch 2 times, most recently from 72634ac to 424c85a Compare January 17, 2018 19:34
@guardbotmk3
Copy link
Author

OK, Bool has been excised from the places where it was used previously, and the C++ Backend has been updated as well. I can change the name of the line numbering flag if you'd like, but at least it's not a boolean now.

@andreasabel
Copy link
Member

Thanks for the change.

Er, sorry to harp on this, but LineNumber sounds like you are passing a line number. Could it be something like HaveLineNumbers, HaveLocations, HaveLoc, HavePositions, HavePos which indicates a flag?

@guardbotmk3
Copy link
Author

OK, I named it RecordPositions. I even renamed all the 'ln' variables rp. Thanks grep and sed.

@guardbotmk3
Copy link
Author

The Travis failure is the same one that I've seen before about GHC versions. The 2 non-broken versions passed.

This reverts commit a65a265.

The location tracking of cup was really awful in 0.11a, so much so that tracking
locations in the abstract syntax tree is basically impossible. Revert this
commit and add a comment about the minimal version required when line numbering
is requested. Verified that cup 0.11a is supported without line numbering.
Adds line numbers to the generated Absyn files with -l
Adds cup support to record the line numbers as the Absyn tree is built.
When compiled with line number support, the symbolFactory argument must now be
supplied to the parser. All of the examples have been updated to reflect this.
Later cup versions complain that the constructor without a symbol factory is
deprecated. Without line numbering, passing the SymbolFactory to the constructor
doesn't hurt anything.
Use datatypes instead of booleans. It makes it much more clear what the
functions are expecting. Line numbering especially, as it needs to be threaded
troughout for both C++ and Java.
@guardbotmk3
Copy link
Author

I re-ran the tests and encountered some errors, that I've cleaned up with this push. Everything should be in good order.

@andreasabel andreasabel merged commit 666a9f7 into BNFC:master Jan 18, 2018
@andreasabel
Copy link
Member

Thanks, I merged this.
I resolved some conflicts with my own patches done for #221, which I pushed now.

@andreasabel andreasabel added this to the 2.8.2 milestone Nov 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants