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

fix: #2171 #2314

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions .vscode/launch.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file doesn't look like it should be added in this PR.
(I'm not familiar with VSCode and how this should work. Perhaps someone else can comment?)

Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
{
// Use IntelliSense to learn about possible attributes.
// Hover to view descriptions of existing attributes.
// For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
"version": "0.2.0",
"configurations": [
{
"name": "(gdb) Attach",
"type": "cppdbg",
"request": "attach",
"program": "enter program name, for example ${workspaceFolder}/a.exe",
"MIMode": "gdb",
"miDebuggerPath": "/path/to/gdb",
"setupCommands": [
{
"description": "Enable pretty-printing for gdb",
"text": "-enable-pretty-printing",
"ignoreFailures": true
},
{
"description": "Set Disassembly Flavor to Intel",
"text": "-gdb-set disassembly-flavor intel",
"ignoreFailures": true
}
]
},
{
"name": "(Windows) Launch",
"type": "cppvsdbg",
"request": "launch",
"program": "enter program name, for example ${workspaceFolder}/a.exe",
"args": [],
"stopAtEntry": false,
"cwd": "${fileDirname}",
"environment": [],
"console": "externalTerminal"
}
]
}
24 changes: 13 additions & 11 deletions verible/verilog/analysis/descriptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,16 @@ struct LintConfigParameterDescriptor {
std::string description;
};

struct LintRuleDescriptor {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean to delete this?

LintRuleId name; // ID/name of the rule.
absl::string_view topic; // section in style-guide
std::string desc; // Detailed description.
std::vector<LintConfigParameterDescriptor> param;
};

} // namespace analysis
} // namespace verilog

#endif // VERIBLE_VERILOG_ANALYSIS_DESCRIPTIONS_H_
std::string format_long_description(const std::string& description) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this new function used anywhere? If so, you may declare it in a .h file, but define it in the corresponding .cc file.

const size_t max_length = 80;
std::string formatted_desc;
size_t start = 0;

while (start < description.size()) {
size_t end = std::min(start + max_length, description.size());
formatted_desc += description.substr(start, end - start) + "\n ";
start = end;
}

return formatted_desc;
}
9 changes: 8 additions & 1 deletion verible/verilog/analysis/verilog-linter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,8 @@ absl::Status PrintRuleInfo(std::ostream *os,
}

void GetLintRuleDescriptionsHelpFlag(std::ostream *os,
absl::string_view flag_value) {
absl::string_view flag_value,
bool verbose) {
// Set up the map.
auto rule_map = analysis::GetAllRuleDescriptions();
for (const auto &rule_id : analysis::kDefaultRuleSet) {
Expand All @@ -380,9 +381,15 @@ void GetLintRuleDescriptionsHelpFlag(std::ostream *os,
*os << status.message();
return;
}

// If verbose flag is set, print example for the rule
if (verbose) {
*os << "\nExample: " << rule.second.GetExample() << "\n"; // Assuming rule.second has GetExample()
}
}
}


void GetLintRuleFile(std::ostream *os, const LinterConfiguration &config) {
// This rule bundle contains only a list of enabled rules. There
// are also no param's defined (an empty string), unless the user
Expand Down
72 changes: 41 additions & 31 deletions verible/verilog/preprocessor/verilog-preprocess.cc
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you missing the change to the verilog-preprocess.h that includes the new condition_stack_ class member and AnnotatePreprocessorDirectives method?

Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@
#include "verible/verilog/parser/verilog-parser.h" // for verilog_symbol_name()
#include "verible/verilog/parser/verilog-token-enum.h"

std::string GenerateConditionComment(const std::vector<std::string>& stack) {
if (stack.empty()) return "";
return " // " + absl::StrJoin(stack, " && ");
}

namespace verilog {

using verible::TokenGenerator;
Expand Down Expand Up @@ -492,13 +497,8 @@ absl::Status VerilogPreprocess::HandleUndef(
}

absl::Status VerilogPreprocess::HandleIf(
const TokenStreamView::const_iterator ifpos, // `ifdef, `ifndef, `elseif
const TokenStreamView::const_iterator ifpos,
const StreamIteratorGenerator &generator) {
if (!config_.filter_branches) { // nothing to do.
preprocess_data_.preprocessed_token_stream.push_back(*ifpos);
return absl::OkStatus();
}

auto macro_name_extract = ExtractMacroName(generator);
if (!macro_name_extract.ok()) {
return macro_name_extract.status();
Expand All @@ -509,58 +509,68 @@ absl::Status VerilogPreprocess::HandleIf(
const bool name_is_defined = defs.find(macro_name->text()) != defs.end();
const bool condition_met = (name_is_defined ^ negative_if);

// Update condition stack for annotation
if (negative_if) {
condition_stack_.push_back("!" + std::string(macro_name->text()));
} else {
condition_stack_.push_back(std::string(macro_name->text()));
}

// Annotate with condition
const std::string comment = GenerateConditionComment(condition_stack_);
preprocess_data_.preprocessed_token_stream.push_back(
verible::TokenInfo(PP_ifdef, (*ifpos)->text() + comment));

if ((*ifpos)->token_enum() == PP_elsif) {
if (conditional_block_.size() <= 1) {
preprocess_data_.errors.emplace_back(**ifpos, "Unmatched `elsif");
return absl::InvalidArgumentError("Unmatched `else");
}
if (!conditional_block_.top().UpdateCondition(**ifpos, condition_met)) {
preprocess_data_.errors.emplace_back(**ifpos, "`elsif after `else");
preprocess_data_.errors.emplace_back(conditional_block_.top().token(),
"Previous `else started here.");
return absl::InvalidArgumentError("Duplicate `else");
return absl::InvalidArgumentError("Unmatched `elsif");
}
conditional_block_.top().UpdateCondition(**ifpos, condition_met);
} else {
// A new, nested if-branch.
const bool scope_enabled = conditional_block_.top().InSelectedBranch();
conditional_block_.push(BranchBlock(scope_enabled, condition_met, **ifpos));
conditional_block_.push(BranchBlock(conditional_block_.top().InSelectedBranch(),
condition_met, **ifpos));
}
return absl::OkStatus();
}


absl::Status VerilogPreprocess::HandleElse(
TokenStreamView::const_iterator else_pos) {
if (!config_.filter_branches) { // nothing to do.
preprocess_data_.preprocessed_token_stream.push_back(*else_pos);
return absl::OkStatus();
}

if (conditional_block_.size() <= 1) {
preprocess_data_.errors.emplace_back(**else_pos, "Unmatched `else");
return absl::InvalidArgumentError("Unmatched `else");
}

if (!conditional_block_.top().StartElse(**else_pos)) {
preprocess_data_.errors.emplace_back(**else_pos, "Duplicate `else");
preprocess_data_.errors.emplace_back(conditional_block_.top().token(),
"Previous `else started here.");
return absl::InvalidArgumentError("Duplicate `else");
// Negate the last condition for annotation
if (!condition_stack_.empty()) {
condition_stack_.back() = "!" + condition_stack_.back();
}
const std::string comment = GenerateConditionComment(condition_stack_);
preprocess_data_.preprocessed_token_stream.push_back(
verible::TokenInfo(PP_else, (*else_pos)->text() + comment));

conditional_block_.top().StartElse(**else_pos);
return absl::OkStatus();
}


absl::Status VerilogPreprocess::HandleEndif(
TokenStreamView::const_iterator endif_pos) {
if (!config_.filter_branches) { // nothing to do.
preprocess_data_.preprocessed_token_stream.push_back(*endif_pos);
return absl::OkStatus();
}

if (conditional_block_.size() <= 1) {
preprocess_data_.errors.emplace_back(**endif_pos, "Unmatched `endif");
return absl::InvalidArgumentError("Unmatched `endif");
}

// Annotate with resolved condition stack
const std::string comment = GenerateConditionComment(condition_stack_);
preprocess_data_.preprocessed_token_stream.push_back(
verible::TokenInfo(PP_endif, (*endif_pos)->text() + comment));

conditional_block_.pop();
if (!condition_stack_.empty()) {
condition_stack_.pop_back();
}
return absl::OkStatus();
}

Expand Down
26 changes: 26 additions & 0 deletions verible/verilog/preprocessor/verilog-preprocess_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1035,3 +1035,29 @@ TEST(VerilogPreprocessTest,

} // namespace
} // namespace verilog

TEST(PreprocessorAnnotationTest, AnnotatesConditionals) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice test!
I suggest adding tests to cover ifndef and elsif as well.

const std::string input = R"(
`ifdef FOO
`ifdef BAR
code
`endif
`else
other_code
`endif
)";

const std::string expected_output = R"(
`ifdef FOO // FOO
`ifdef BAR // FOO && BAR
code
`endif // FOO && BAR
`else // !FOO
other_code
`endif // FOO
)";

VerilogPreprocess preprocess(/*config=*/{});
auto output = preprocess.AnnotatePreprocessorDirectives(input);
EXPECT_EQ(output, expected_output);
}
Loading