sverilogparse: accept empty line-comments (//\n)#2
Open
robtaylor wants to merge 1 commit into
Open
Conversation
`skip_whitespace_and_comment`'s line-comment matcher used
`is_not("\r\n")`, which in nom 7 requires at least one character —
so `//\n` (a `//` immediately followed by a newline, no body text)
failed parsing. This is the dominant shape in human-written file
headers (license blocks, design notes, IP integration comments
with empty separator lines between paragraphs), so consumers
either had to strip the blank-comment lines or switch the whole
header to `/* … */` block comments.
Swap to `take_while(|c| c != b'\r' && c != b'\n')` which accepts
zero-length matches. `is_not` is no longer needed in this module,
so drop it from the import block.
New test `test_header_comments_with_empty_lines` covers the
empty-comment-line case plus a multi-paragraph header layout
matching how foundry IP cells typically ship their copyright +
license + integration-notes blocks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
`skip_whitespace_and_comment`'s line-comment matcher used `is_not("\r\n")`, which in nom 7 requires at least one character. So `//\n` — a `//` immediately followed by a newline, no body text — failed parsing.
This is the dominant shape in human-written file headers (license blocks, design notes, IP integration comments with empty separator lines between paragraphs), so consumers either had to strip the blank-comment lines or switch the entire header to `/* … /` block comments. Bit me writing the ANSI test fixture for #1 (worked around by removing blank `//` lines from the fixture); bit a downstream Jacquard consumer next (worked around by switching their shim's header to `/ */` block comments).
Fix
Swap to `take_while(|c| c != b'\r' && c != b'\n')` which accepts zero-length matches. `is_not` is no longer used in this module, so drop it from the import block.
Tests
New `test_header_comments_with_empty_lines` covers the empty-comment-line case plus a multi-paragraph header layout matching how foundry IP cells typically ship their copyright + license + integration-notes blocks. All 7 pre-existing tests stay green.
Downstream
Jacquard's submodule pointer will need bumping once this lands — straightforward, no Jacquard-side code change needed.