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

Add support for a single quote in a character literal and a double qu… #57

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

skaupper
Copy link

@skaupper skaupper commented Jul 3, 2023

As discussed in #56, this PR adds support for single quotes in a character literal (''') and double quotes in a string literal ("""", "foo""", etc.).

As soon as #56 is merged, this PR will get additional test cases (like character'(''')) to avoid conflicts with qualified expressions.

yield previousToken
tokenKind = cls.TokenKind.OtherChars

start = SourceCodePosition(row, column, absolute)
Copy link
Author

Choose a reason for hiding this comment

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

The following if-elif code block is now present about eight times in this function. What are your thoughts on replacing those with calls to a separate function?

The variables buffer, previousToken, tokenKind etc. would have to be passed by reference in order to support yielding tokens out of it. But I do not really know a simple, non-hacky way to pass enumerations by reference (other than wrapping it manually in a temporary object/dict ofc).

tokenKind = cls.TokenKind.OtherChars
else:
continue
continue # TODO: Merge with changes from #56!
Copy link
Author

Choose a reason for hiding this comment

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

This if-block will be merged with the changes made by #56.

Copy link
Owner

Choose a reason for hiding this comment

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

#56 was merged. What's next for this PR? Anything I can do?

Copy link
Author

Choose a reason for hiding this comment

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

As soon as I have some spare time I will rebase this branch. Since it touches the same if-branches in the parser as #56 I will have a look if something can be cleaned up.

I would appreciate your thoughts about my other comment, though.

@skaupper skaupper force-pushed the bugfix/vhdl_character_and_string_literals branch from 8cd5ca2 to 63fb610 Compare July 3, 2023 09:25
@Paebbels Paebbels mentioned this pull request Jul 8, 2023
@Paebbels
Copy link
Owner

Oh. looks like I got distracted from that one ...

@skaupper skaupper force-pushed the bugfix/vhdl_character_and_string_literals branch from 63fb610 to 479005d Compare August 31, 2023 08:36
@skaupper skaupper marked this pull request as ready for review August 31, 2023 08:40
@skaupper
Copy link
Author

As far as I am concerned, this PR could be merged. Do you have any objections?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants