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

Improve parseability of type and constant string representations #649

Merged
merged 10 commits into from
Nov 5, 2022

Conversation

paulsc96
Copy link
Contributor

@paulsc96 paulsc96 commented Nov 3, 2022

Overview

Enable TypePrinter and various toString()s to produce more parseable output by optionally emitting well-formed SystemVerilog expressions in more situations.

All changes below except 3. keep existing stringifications unchanged by default. All existing code remains compatible through optional fields and parameters.

These changes do not guarantee type or constant parseability, but cover some simple-to-fix gaps in Slang's already quite parseable stringifications.

Changes

TypePrinter

  1. Add the skipScopedTypeNames option to TypePrinter to omit suffixed type names from container types.
  2. Add the fullEnumTypeoption to TypePrinter to include enum base types.

ConstantValue and SVInt

  1. Use assignment patterns ('{...} ) instead of simple brackets ([...]) for container constants.
  2. Expose abbreviateThresholdBits to the end user of ConstantValue.toString() and callees to control or suppress integer vector abbreviation as needed.
  3. Add the optional exactUnknowns parameter to ConstantValue.toString() and callees to ensure a reversible representation of integer vectors with unknown bits.

@paulsc96 paulsc96 marked this pull request as draft November 3, 2022 23:59
Copy link
Owner

@MikePopoloski MikePopoloski 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 diff. I have a few minor comments but happy to land this once you address them.

source/numeric/ConstantValue.cpp Outdated Show resolved Hide resolved
include/slang/ast/types/TypePrinter.h Show resolved Hide resolved
source/numeric/SVInt.cpp Outdated Show resolved Hide resolved
@MikePopoloski
Copy link
Owner

Also as you can probably tell from the failed tests, there are a bunch of eval tests that need to be updated for your ConstantValue toString change to use '{} but I think it's fine to not make that an option and just update the tests instead.

@paulsc96 paulsc96 force-pushed the master branch 2 times, most recently from 08d5eaf to 04caea4 Compare November 4, 2022 13:12
@codecov
Copy link

codecov bot commented Nov 4, 2022

Codecov Report

Merging #649 (52d6532) into master (768c295) will decrease coverage by 0.03%.
The diff coverage is 69.44%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #649      +/-   ##
==========================================
- Coverage   91.23%   91.19%   -0.04%     
==========================================
  Files         185      185              
  Lines       42042    42200     +158     
==========================================
+ Hits        38357    38486     +129     
- Misses       3685     3714      +29     
Impacted Files Coverage Δ
include/slang/numeric/ConstantValue.h 100.00% <ø> (ø)
include/slang/numeric/SVInt.h 98.50% <ø> (ø)
source/ast/types/TypePrinter.cpp 67.76% <64.28%> (-0.15%) ⬇️
source/numeric/SVInt.cpp 98.79% <71.42%> (ø)
source/numeric/ConstantValue.cpp 72.97% <73.33%> (+0.06%) ⬆️
source/ast/symbols/ParameterSymbols.cpp 80.83% <0.00%> (-4.03%) ⬇️
include/slang/ast/SemanticFacts.h 50.00% <0.00%> (-3.85%) ⬇️
source/ast/ASTSerializer.cpp 63.81% <0.00%> (-2.14%) ⬇️
source/ast/symbols/ClassSymbols.cpp 94.00% <0.00%> (-0.19%) ⬇️
include/slang/ast/Symbol.h 100.00% <0.00%> (ø)
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 768c295...52d6532. Read the comment docs.

@paulsc96
Copy link
Contributor Author

paulsc96 commented Nov 4, 2022

I chose to make 3. optional now and keep the existing default behavior, maintaining full compatibility.

@paulsc96 paulsc96 marked this pull request as ready for review November 4, 2022 15:06
@MikePopoloski MikePopoloski merged commit 84bff64 into MikePopoloski:master Nov 5, 2022
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