Some miscellaneous improvements to dump flags #859
Conversation
123cf84 to
7de36ad
Compare
- Include headers and footers when dumping to files, just like when dumping to stdout - Only overwrite a file the first time we dump to it in a run. Subsequent dumps append to the existing file. This means -dall=foo.txt collects everything instead of just the last dump.
so have them overwrite files to prevent double-dumps.
Some splitIf testcases compile a submodule. Previously, the submodule splitIf dump was overwritten so the testing code only saw the top-level dump. That changed when we started accumulating dumps, so we redirect each module's dump to a different file with %m.
7de36ad to
48ce136
Compare
quark17
left a comment
There was a problem hiding this comment.
Awesome. Just a couple comments.
| "You may substitute -KILL for -d" ++ | ||
| " to stop compilation after the named pass", | ||
| "", | ||
| "-dall supports filename arguments including digraphs", |
There was a problem hiding this comment.
Since -dall was removed from externalFlags below, that not only removed the parsing, but also removed the display of the description in the help message. This line below the digraphs is now the only place that mentions -dall to the user at all. So I think it needs to say more than just that -dall supports arguments, but to also say what -dall is even is. Maybe something like, "-dall enables dumping after every pass. -dall also supports an optional filename including digraphs. If a filename is given for an individual pass, it takes precedence over the -dall filename."
| then appendFileCatch | ||
| else writeFileCatch | ||
| -- Add a header and footer to -dall dumps in case they end up in the same file. | ||
| let a' = if hasDumpStrict flags d then a else header ++ a ++ footer |
There was a problem hiding this comment.
One, you might want to replace hasDumpStrict flags d with isSpecificDump.
But, two, I think we should always write the header and footer. It's possible for specific dumps to write to the same file -- the current behavior means that they'll be appended without any header/footer delimiters. (We might even consider including the names in the headers.)
There was a problem hiding this comment.
Ah, I see that the testsuite uses -dparsed in bsc.syntax/bh_parse_pretty/, for example, and expects to be able to run BSC directly on the file. So if we added headers and footers, then the testsuite would need to be able to remove them. (Is that so bad?)
| let isSpecificDump = hasDumpStrict flags d | ||
| -- Don't allow explicit parsed dumps to accumulate to avoid a double dump | ||
| -- from the double parse (first for dependencies and then for code). | ||
| let doubleParsedPasses = [DFvpp, DFbsvlex, DFparsed] |
There was a problem hiding this comment.
I'm not wholly satisfied with the handling of parser passes. I guess it's OK for now. If someone didn't provide enough digraphs, multiple passes can be written to the same file; except that the parser passes would overwrite the file. Would it be better to give separate names to the dependency parser stages (like how you did for wrapper stages)? Or just live with multiples in the file, if you ran BSC with the -u flag.
Was this added because of uses in the testsuite?
There are two headline improvements here:
%sdigraph in dump file names to get the compiler pass included in the output file name.-dallto take a file argument as well.To get
-dall=file(with no digraphs) to work, I had the compiler start appending to dump files after the first write.That turned out to be tricky because of how we were using dump flags in the testsuite. In one case I redirected dumps with
%mso that tests didn't see the dumps of multiple modules mixed together. The other issue was how our double-parsing (once for dependencies and once for compilation) interacted with the dump code. I decided the least-surprising behavior that worked with our tests was to have the parsing stages be an exception to the append after first write rule (when they are being explicitly dumped).