Increase width's width for trace values
#2172
Conversation
adpaco-aws
changed the title
width for trace valueswidth's width for trace values
| @@ -35,6 +35,8 @@ use std::os::unix::process::ExitStatusExt; | |||
| use std::path::PathBuf; | |||
| use std::process::{Child, ChildStdout}; | |||
|
|
|||
| const RESULT_ITEM_PREFIX: &str = " {\n \"result\":"; | |||
There was a problem hiding this comment.
Should we use regex instead or will this run all the time? I'm worried this is very fragile.
There was a problem hiding this comment.
There were a few things I considered here, for example trimming the string without delimiters (so we can check against "{\"result\":"). The problem is that trimming that string could impact performance: the one that caused the crash in #2169 is 921K lines (33MB). So I went for the most performant check (this one), which should run all the time.
| // shouldn't be hard to debug with that information. The same strategy | ||
| // can be used for other `ParserItem` variants, but they're normally | ||
| // easier to debug. | ||
| if string_without_delimiter.starts_with(RESULT_ITEM_PREFIX) { |
There was a problem hiding this comment.
Have you considered changing the signature of this function to Result and in the case of failure you can add the context that we failed on (i.e., what was the item that we were trying to parse).
There was a problem hiding this comment.
We don't know which item we're trying to parse, that's why we use the ParserItem enum. I can change the signature to use Result<ParserItem> but I don't know what to with the result other than unwrap().
There was a problem hiding this comment.
I made the change but then dropped it because the new case would return a ResultStruct. I'm open to suggestions here.
There was a problem hiding this comment.
Could you use expect() and add to the error message the string that we failed to parse.
There was a problem hiding this comment.
Thanks for explaining the issue offline. I'll resolve this comment.
There was a problem hiding this comment.
I cannot get rid of #[allow(unused)] even if use ResultStruct.result in the test.
adpaco-aws
force-pushed
the
fix-issue-2169
branch
from
b54e4a1 to
9e009af
Compare
| /// Checks that a valid CBMC "result" item can be deserialized into a | ||
| /// `ParserItem` or `ResultStruct`. | ||
| #[test] | ||
| fn check_result_deserialization_works() { |
Description of changes:
TraceValue.widthto beOption<u32>instead ofOption<u8>(this fixes Unhandled variant in ParserItem JSON #2169).parse_itemto provide better error messages forParserItem::Result.Resolved issues:
Resolves #2169
Call-outs:
Testing:
How is this change tested? CI + manually tested on example from Unhandled variant in ParserItem JSON #2169 (the example takes a few minutes to run, which is why I'm not including it in the regression).
Is this a refactor change? No.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.