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

Bugfixes for preprocess(), plus organise and extend tests. #61

Merged
merged 62 commits into from
Aug 1, 2022

Conversation

DaveMcEwan
Copy link

@DaveMcEwan DaveMcEwan commented Jul 20, 2022

The changes in this PR are:

  • Rename existing tests with descriptive names, e.g. "test1" -> "ifdef_undefined".
    The diff looks awful, so please have a look through testcases/*.sv and testcases/expected/*.sv
    to see the proposed testing regime.
  • Put expected results into separate files.
    This makes it easier to update tests without worrying about breaking checks on trailing whitespace
    because lots of people (including me) have their editor setup to strip that on write.
  • Fix bugs with extraneous whitespace from text macro expansion.
    This is basically ppWhitespace done more thoroughly - sorry for the partial work!
    Leading spaces/tabs should not be included in output, but a line continuation can start whitespace
    to be included.
    Whitespace attached to the macro usage node should be output, rather than outputting a fixed 1 or
    2 spaces.
  • Fix bugs around __FILE__ and __LINE__.
    A well-known vendor tests if these are defined, presumably for backward compatibility.
    The fix treats them as always defined, and prevents parse error when an unevaluated define sets them.
  • Fix bugs with undef,undefineall being removed from output where defines are left in.
  • Allow closure of Unexpected include error #48 (copy of Unexpected include error svlint#77).
    The reported testcases can be interpreted to be out-of-spec, because the quotes are expanded first.
    See IEEE1800-2017 page 680, two paragraphs.
  • Protect against infinite include recursion.
    Raise an Err(ExceedRecursiveLimit) instead of stack overflow.
    This adds a new argument include_depth to preprocess_str(), breaking backwards compatibility.

This is already quite a large PR, so I'd like to leave fixes for #9 and #57 for a separate one.

- Redefinitions are silently ignored.
- ifdef/ifndef will always treat them as defined.
- Enables compatibility with code from a silicon vendor I can think of.
- NOTE: Whitespace bugs around endif/undef are observable in testcases.
- Replacing with more thorough tests.
- Similarly to how `define is left in-place, `undef and `undefine are also
  left in-place.
- Corresponding fixes to macro_LINE and macro_FILE.
- These directives were not in any testcases before this branch.
1. It's much less visual noise.
2. Preparation for change to `preprocess()` and `preprocess_str()` arguments.
- Ignored by default.
- Causes stack overflow.
- New argument `include_depth` on `preprocess_str()`.
- Existing function `preprocess()` is now a wrapper for private function
  `preprocess_inner()` which uses `include_depth`.
- Commented arguments on all uses of `preprocess()` and `preprocess_str()`.
- Test `include_recursive` is fixed and not ignored.
- Without recursion limit, stack overflow occurs which crashes svls.
- Illustration of dalance/svlint#77
- That's the same as dalance#48
- A,B show what doesn't work, vaguely defined on page 680.
- C,D show what does work, using example on page 680.
@DaveMcEwan DaveMcEwan marked this pull request as ready for review July 26, 2022 20:31
@DaveMcEwan DaveMcEwan changed the title WIP Tidy, correct, and extend tests for preprocess(). Bugfixes for preprocess(), plus organise and extend tests. Jul 28, 2022
@dalance
Copy link
Owner

dalance commented Aug 1, 2022

LGTM
Thanks!

@dalance dalance merged commit aabc9aa into dalance:master Aug 1, 2022
@DaveMcEwan DaveMcEwan deleted the ppTests branch November 9, 2022 10:51
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.

2 participants