resolve() touch ups#286
Merged
Merged
Conversation
ktbarrett
force-pushed
the
resolve-touch-ups
branch
from
3935b6c to
f10f4a9
Compare
ktbarrett
force-pushed
the
resolve-touch-ups
branch
from
f10f4a9 to
406218c
Compare
The WEAK resolve method used to map W to X and pass U/X/Z/- through unchanged. That left WEAK as a "best effort" pass that quietly returned non-resolvable values, which is at odds with the obvious reading of "the method either resolves to 0/1 or fails." After this change, WEAK accepts only 0/1/L/H (the strengths that have a meaningful 0/1 interpretation) and throws std::invalid_argument on everything else. Callers that want X/Z/U/W/- to silently pick a value already have ZEROS/ONES/RANDOM. Callers that want strict 0/1 already have ERROR. WEAK now slots between them as "accept HDL weak strengths, reject metavalues."
is_resolvable() previously hard-coded WEAK. This makes it not particularly useful as a predicate for "will resolve() throw?" Also it wasn't a good predicate for WEAK either since WEAK still returned a value when it got an unresolvable metavalue.
Previously this returned Self, now since all methods resolve to 0/1 this can return Bit and BitArray/BitVector. These types have implicit casts to bool and Bit has an implicit int cast. This also is a good jumping off point to cast to Unsigned, Signed, Sfixed, Ufixed, or Float.
ktbarrett
force-pushed
the
resolve-touch-ups
branch
from
406218c to
a6624d3
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #286 +/- ##
==========================================
- Coverage 92.05% 91.88% -0.18%
==========================================
Files 23 23
Lines 1309 1318 +9
Branches 316 330 +14
==========================================
+ Hits 1205 1211 +6
- Misses 43 45 +2
- Partials 61 62 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
resolve(method) now returns std::optional<Bit> (Logic/Bit) or std::optional<Array<Bit, R>> / std::optional<Vector<Bit>> (LogicArrayMixin). This was done because the previous method required a two-pass approach which is usually a performance smell.
ktbarrett
force-pushed
the
resolve-touch-ups
branch
from
a6624d3 to
ea7d2bb
Compare
Owner
Author
|
The remaining coverage holes will be covered by upstream PRs. |
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.
Supersedes #283.
I decided that we are going to handle metavlue resolving in the handles and not on the datatypes themselves since that makes for some truly strange semantics. But I did keep some good ideas from the old PR:
WEAKresolve method now throws on metavlues.Logic.is_resolvableandLogicArray.is_resolvablenow take a resolve method.Logic.resolveandLogicArray.resolvenow return aBitandBitArray, respectively.Python is left unchanged to make this mergeable now.