Removed the limit on the number of traces in dumpVCD
#3083
Conversation
| labels = "!" : map go [1..] | ||
| where | ||
| go 0 = "" | ||
| go n = chr ( 33 + n `mod` k) : go (n `div` k) | ||
| k = 126-33+1 |
There was a problem hiding this comment.
How about this instead:
| labels = "!" : map go [1..] | |
| where | |
| go 0 = "" | |
| go n = chr ( 33 + n `mod` k) : go (n `div` k) | |
| k = 126-33+1 | |
| labels = concatMap (\s -> map (snoc s) alphabet) ([]: labels) | |
| where | |
| alphabet = map chr [33..126] |
You'll need to import
import Data.List.Extra (snoc)There was a problem hiding this comment.
This is one fancy way of doing things.
Are there specific things that make it better? I do agree it's better, but there might be stuff I am missing.
There was a problem hiding this comment.
If the alphabet were "abc", this is what it would generate:
>>> import Text.Show.Pretty
>>> pPrint $ P.take 20 labels
[ "a"
, "b"
, "c"
, "aa"
, "ab"
, "ac"
, "ba"
, "bb"
, "bc"
, "ca"
, "cb"
, "cc"
, "aaa"
, "aab"
, "aac"
, "aba"
, "abb"
, "abc"
, "aca"
, "acb"
]It is almost how a base-n number system would work, except that 0 is also significant in leading positions. It just seems like a clean numbering to me.
It's also more Haskelly if you ask me :-).
There was a problem hiding this comment.
The recursion step of the algorithm in words is roughly this:
To form all labels of length up to n+1 we take all labels up to a length n and for all of those labels and all letters of the alphabet, we append the letter of the alphabet to the label. Also, for each letter of the alphabet, include just that letter as a label (because by increasing the length of all labels we lost the labels of length 1 plus we clearly need to start somewhere).
| format 1 label (0,0) = '0': label <> "\n" | ||
| format 1 label (0,1) = '1': label <> "\n" | ||
| format 1 label (1,_) = 'x': label <> "\n" |
There was a problem hiding this comment.
This file seems to use ++ for concatenating strings, in the interest of consistency I'd like to see that maintained here.
There was a problem hiding this comment.
Looking at these lines, why do we even add a newline? It isn't there a few lines later:
format n label (mask,val) =
"b" ++ map digit (reverse [0..n-1]) ++ " " ++ labelThere was a problem hiding this comment.
You're completely right, they don't need a newline! It's already handled in two Text.intercalate "\n"s. Let's remove it.
marijn-qbaylogic
force-pushed
the
fix-3082-dumpvcd-trace-limit
branch
from
1c910be to
8c6d1f8
Compare
DigitalBrains1
left a comment
DigitalBrains1
left a comment
There was a problem hiding this comment.
I'd like to stress that newlines are necessary after one-bit signals; it's just that we emitted two of them.
Please also add something similar to the PR cover letter (the first message in the PR).
| @@ -0,0 +1 @@ | |||
| FIXED: Removed the limit on the number of traces in `dumpVCD`. Also removed unnecessary newlines after 1-bit signals. [#3082](https://github.com/clash-lang/clash-compiler/issues/3082) | |||
There was a problem hiding this comment.
| FIXED: Removed the limit on the number of traces in `dumpVCD`. Also removed unnecessary newlines after 1-bit signals. [#3082](https://github.com/clash-lang/clash-compiler/issues/3082) | |
| FIXED: Removed the limit on the number of traces in `dumpVCD`. Also removed unnecessary double newlines after 1-bit signals. [#3082](https://github.com/clash-lang/clash-compiler/issues/3082) |
There was a problem hiding this comment.
A different way to phrase it would be
| FIXED: Removed the limit on the number of traces in `dumpVCD`. Also removed unnecessary newlines after 1-bit signals. [#3082](https://github.com/clash-lang/clash-compiler/issues/3082) | |
| FIXED: Removed the limit on the number of traces in `dumpVCD`. Also removed unnecessary blank lines after 1-bit signals. [#3082](https://github.com/clash-lang/clash-compiler/issues/3082) |
I think that's even more clear.
| offensiveNames = filter (any (not . printable)) traceNames | ||
|
|
||
| labels = map chr [33..126] | ||
| -- Generate labels like reversed digits: 0:[1,2,01,11,21,02,12,22,001,...] |
There was a problem hiding this comment.
| -- Generate labels like reversed digits: 0:[1,2,01,11,21,02,12,22,001,...] | |
| -- Generate labels almost like digits: if the alphabet were "012", generate | |
| -- [0,1,2,00,01,02,10,11,12 ..] |
Although frankly it can do without the comment; it's just that the current comment is wrong. Feel free to just omit it altogether.
`dumpVCD` was limited to 93 traces, because it could only generate single character trace identifier codes. The codes are now generated ad infinitum, and the restriction has been removed. Additionally, single bit traces were logged with an extra newline, causing empty lines in the VCD. This has been removed as well. Fixes: #3082 (cherry picked from commit fb8f164)
`dumpVCD` was limited to 93 traces, because it could only generate single character trace identifier codes. The codes are now generated ad infinitum, and the restriction has been removed. Additionally, single bit traces were logged with an extra newline, causing empty lines in the VCD. This has been removed as well. Fixes: #3082 (cherry picked from commit fb8f164) Co-authored-by: marijn-qbaylogic <marijn@qbaylogic.com>
dumpVCDwas limited to 93 traces, because it could only generate single character trace identifier codes. The codes are now generated ad infinitum, and the restriction has been removed.Additionally, single bit traces were logged with an extra newline, causing empty lines in the VCD. This has been removed as well.
Fixes: #3082