Skip to content
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

move ViolationHandler to common #1080

Merged
merged 1 commit into from
Nov 9, 2021

Conversation

wsipak
Copy link
Collaborator

@wsipak wsipak commented Nov 2, 2021

This is a refactoring step before introducing Reviewdog Diagnostic Format in Verible:
#1018

ViolationHandler is the interface which is supposed to be implemented by ViolationPrinter, RDFormat printer, and other potential handlers.
This isn't specific to Verilog and thus we place it in common.

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2021

Codecov Report

Merging #1080 (1289f4f) into master (0d1c936) will decrease coverage by 0.00%.
The diff coverage is 93.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1080      +/-   ##
==========================================
- Coverage   92.91%   92.91%   -0.01%     
==========================================
  Files         334      335       +1     
  Lines       22196    22192       -4     
==========================================
- Hits        20624    20620       -4     
  Misses       1572     1572              
Impacted Files Coverage Δ
common/analysis/lint_rule_status.cc 96.61% <ø> (-0.22%) ⬇️
verilog/analysis/verilog_linter.cc 92.47% <ø> (-0.28%) ⬇️
verilog/tools/ls/verible-lsp-adapter.cc 100.00% <ø> (ø)
verilog/tools/lint/verilog_lint.cc 95.23% <85.71%> (ø)
common/analysis/violation_handler.cc 93.12% <93.12%> (ø)
common/analysis/lint_rule_status.h 100.00% <100.00%> (ø)
common/analysis/violation_handler.h 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d1c936...1289f4f. Read the comment docs.

Copy link
Collaborator

@hzeller hzeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Just one missing dependency.

@@ -227,14 +227,13 @@ cc_library(
"//common/analysis:text_structure_linter",
"//common/analysis:token_stream_lint_rule",
"//common/analysis:token_stream_linter",
"//common/strings:diff",
"//common/analysis:violation_handler",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the verilog_linter_test below also needs this dependency now.

@@ -34,6 +34,7 @@
#include "absl/status/statusor.h"
#include "absl/strings/match.h"
#include "absl/strings/string_view.h"
#include "common/analysis/violation_handler.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... because of this added include.

@hzeller
Copy link
Collaborator

hzeller commented Nov 8, 2021

If you add the dependency, this can be merged.

@wsipak wsipak force-pushed the wsip/rdformat-refactor branch from 13adf51 to 1289f4f Compare November 8, 2021 17:57
@wsipak
Copy link
Collaborator Author

wsipak commented Nov 8, 2021

Now verilog_linter_test has "//common/analysis:violation_handler" as a dependency. Thank you.

@wsipak wsipak requested a review from hzeller November 8, 2021 18:52
@hzeller
Copy link
Collaborator

hzeller commented Nov 9, 2021

LGTM!

@hzeller hzeller merged commit 83b86f7 into chipsalliance:master Nov 9, 2021
@tgorochowik tgorochowik deleted the wsip/rdformat-refactor branch November 9, 2021 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants