-
-
Notifications
You must be signed in to change notification settings - Fork 83
Improved error parsing when using verilator #491
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
Conversation
Thanks! I'll review it in a few days. |
src/linter/VerilatorLinter.ts
Outdated
@@ -107,42 +107,78 @@ export default class VerilatorLinter extends BaseLinter { | |||
command, | |||
{ cwd: cwd }, | |||
(_error: Error, _stdout: string, stderr: string) => { | |||
let diagnostics: vscode.Diagnostic[] = []; | |||
//let diagnostics: vscode.Diagnostic[] = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to leave the old code in the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/linter/VerilatorLinter.ts
Outdated
|
||
// basically DiagnosticsCollection but with ability to append diag lists | ||
let filesDiag = {}; | ||
let error_warning_counter = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use camelCase for variable name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/linter/VerilatorLinter.ts
Outdated
//let diagnostics: vscode.Diagnostic[] = []; | ||
|
||
// basically DiagnosticsCollection but with ability to append diag lists | ||
let filesDiag = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use Map instead of Object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/linter/VerilatorLinter.ts
Outdated
} | ||
return; | ||
} | ||
this.logger.warn('[verilator] failed to parse error: ' + line); | ||
}); | ||
this.logger.info(`[verilator] ${diagnostics.length} errors/warnings returned`); | ||
this.diagnosticCollection.set(doc.uri, diagnostics); | ||
this.logger.info(`[verilator] ${error_warning_counter} errors/warnings returned`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be something like this instead of counting error/warning.
[...filesDiag.values()].reduce((total, arr) => total + arr.length, 0))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After it's done, Verilator prints something like %Error: Exiting due to 3 error(s) and 2 warning(s)
, which is passed through to the user (well, console). This message is redundant and less informative, thus has been removed entirely.
When / if you look at this, have a look at this, have a look at It's not production-quality code yet and the whole thing turned into a bit of a mess, but main functionality is there. |
@alex-the-new-guy LGTM. Thanks! |
Fixed/improved problem matcher regex, added support for parsing a whole project (theoretically) instead of current file only. Guess that's it.