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.

Sign up for GitHub

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

Jump to bottom

Add end position information to BNFC'Position #463

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add end position information to BNFC'Position #463

wants to merge 3 commits into from

Conversation

aabounegm
Copy link

@aabounegm aabounegm commented Oct 25, 2023 •
edited
Loading

I made an attempt at implementing the feature I asked for. Please let me know if there is anything I can improve on or some style I'm not strictly following (nitpicks are welcome 😄).

Resolves #461

@aabounegm
Copy link
Author

I have tried running the doctests locally but failed for some reason. stack test doesn't seem to run them (I don't have cabal), and installing manually (using stack install doctest) and then running doctest source/src gives me a bunch of errors I couldn't fix, like so:

source\src\BNFC\License.hs:7:1: error:
    Could not find module ‘Data.String.QQ’
    Perhaps you meant Data.String (from base-4.18.1.0)
    Use -v (or `:set -v` in ghci) to see a list of the files searched for.

source\src\BNFC\CF.hs:38:1: error:
    Could not find module ‘BNFC.Par’
    Use -v (or `:set -v` in ghci) to see a list of the files searched for.

source\src\BNFC\CF.hs:39:1: error:
    Could not find module ‘BNFC.Lex’
    Use -v (or `:set -v` in ghci) to see a list of the files searched for.

which also caused a lot of variable not in scope errors. Do you have any suggestions for how to run the doctests?

@aabounegm
Copy link
Author

I believe the tests should be fixed now. @andreasabel when do you think you can take a look?

@deemp deemp mentioned this pull request Dec 29, 2023
Open
9 tasks
@aabounegm
Copy link
Author

Hello @andreasabel, sorry to bother you again, but do you have an estimate for when you might have time to review this?

andreasabel
andreasabel reviewed May 30, 2024
View reviewed changes
source/src/BNFC/Backend/Haskell/CFtoAbstract.hs
@@ -159,6 +178,7 @@ cf2Abstract Options{ lang, tokenText, generic, functor } name cf = vsep . concat
[ [ text $ List.intercalate ", " stdClasses | hasTextualToks || hasData ]
, [ text $ List.intercalate ", " funClasses | fun ]
, [ text $ "Int, Maybe(..)" | defPosition ]
, [ text $ "fmap, fst, snd"]
Copy link
Member

Choose a reason for hiding this comment

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

These functions should only be imported when used (currently, that would be under condition functor), otherwise compilation generates warnings with -Wall.

andreasabel
andreasabel reviewed May 30, 2024
View reviewed changes
source/src/BNFC/Backend/Haskell/CFtoAlex3.hs
, "tokenLineCol :: Token -> (Int, Int)"
, "tokenLineCol = posLineCol . tokenPosn"
, ""
, "-- | Get end line and column of a token."
, "tokenLineColEnd :: Token -> (Int, Int)"
, "tokenLineColEnd t = (l, c + n)"
Copy link
Member

Choose a reason for hiding this comment

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

What if a token spans several lines? Note that the user can define their own tokens that may contain newline characters.

andreasabel
andreasabel requested changes May 30, 2024
View reviewed changes
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.

Thanks for the PR, and sorry for the delay in evaluating this PR.
Did you run the testsuite (in subdirectory testing)?
It seems your patch breaks e.g. the --agda --functor backend.
Also, if we change the functionality of --functor, we will potentially break a lot of user code.
So I suggest to implement the new feature under a new flag, e.g. --position, and explain how it differs from --functor.
When I had a quick look at the code I saw that you assume tokens to only span a single line, but user-defined tokens can also span several lines.

@andreasabel andreasabel added position Concerning position information in parsed AST Haskell/Functor Concerning the Haskell backend with --functor labels May 30, 2024
@aabounegm
Copy link
Author

Thanks for the review, you've raised some good points indeed. Unfortunately, I have gotten rather busy lately and don't think I will be able to address them relatively soon.
About the addition of a new flag, it felt somewhat redundant to have 2 flags with very similar functionality, except one has more information than the other. Wouldn't a major version update be more suitable in your opinion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreasabel andreasabel andreasabel requested changes

Assignees
No one assigned
Labels
Haskell/Functor Concerning the Haskell backend with --functor position Concerning position information in parsed AST
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Store tokens' position range instead of just start position
2 participants
@aabounegm @andreasabel