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

Override style guide citations #817

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

pawelsag
Copy link
Contributor

Fixes: Override style-guide citation links and references #519

@google-cla google-cla bot added the cla: yes All contributors in pull request have signed the CLA with Google. label May 15, 2021
@pawelsag pawelsag requested a review from hzeller May 15, 2021 16:37
@pawelsag pawelsag force-pushed the psagan/override_style_guide branch from c0764fa to 198b208 Compare May 15, 2021 16:41
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.

First quick round of comments.

verilog/analysis/verilog_linter.cc Outdated Show resolved Hide resolved
verilog/analysis/verilog_linter.cc Outdated Show resolved Hide resolved
@@ -332,10 +333,51 @@ absl::Status PrintRuleInfo(std::ostream* os,
return absl::OkStatus();
}

static void AppendCitation(CustomCitationMap& citations,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Output parameters should usually be at the end;

Also it is custom in this code-base to have output parameters pointers instead of references, but given that this is a static helper function used right afterwards, the confusion about it being a reference is probably not that big, so if you really prefer the reference, keep it :)

verilog/analysis/verilog_linter.h Outdated Show resolved Hide resolved
verilog/analysis/verilog_linter_test.cc Outdated Show resolved Hide resolved
verilog/analysis/verilog_linter.cc Outdated Show resolved Hide resolved
verilog/analysis/verilog_linter.cc Outdated Show resolved Hide resolved
verilog/analysis/verilog_linter.cc Outdated Show resolved Hide resolved
verilog/analysis/verilog_linter.cc Outdated Show resolved Hide resolved
@pawelsag pawelsag force-pushed the psagan/override_style_guide branch from b58a5b3 to 565ee92 Compare May 21, 2021 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes All contributors in pull request have signed the CLA with Google.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants