diff --git a/verilog/preprocessor/verilog_preprocess.cc b/verilog/preprocessor/verilog_preprocess.cc index 1bd8601f2..c480fde0e 100644 --- a/verilog/preprocessor/verilog_preprocess.cc +++ b/verilog/preprocessor/verilog_preprocess.cc @@ -192,15 +192,15 @@ absl::Status VerilogPreprocess::HandleTokenIterator( } // Stores a macro definition for later use. -std::unique_ptr -VerilogPreprocess::RegisterMacroDefinition(const MacroDefinition& definition) { +void VerilogPreprocess::RegisterMacroDefinition( + const MacroDefinition& definition) { // For now, unconditionally register the macro definition, keeping the last // definition if macro is re-defined. const bool inserted = InsertOrUpdate(&preprocess_data_.macro_definitions, definition.Name(), definition); - if (inserted) return nullptr; - return std::make_unique(definition.NameToken(), - "Re-defining macro"); + if (inserted) return; + preprocess_data_.warnings.emplace_back( + VerilogPreprocessError(definition.NameToken(), "Re-defining macro")); // TODO(hzeller): multiline warning with 'previously defined here' location } @@ -228,13 +228,13 @@ absl::Status VerilogPreprocess::HandleDefine( preprocess_data_.errors.push_back(*parse_error_ptr); return absl::InvalidArgumentError("Error parsing macro definition."); } + RegisterMacroDefinition(macro_definition); + // For now, forward all definition tokens. - if (auto warning = RegisterMacroDefinition(macro_definition); warning) { - preprocess_data_.warnings.push_back(*warning); - } for (const auto& token : define_tokens) { preprocess_data_.preprocessed_token_stream.push_back(token); } + return absl::OkStatus(); } diff --git a/verilog/preprocessor/verilog_preprocess.h b/verilog/preprocessor/verilog_preprocess.h index 2ef9eff1d..3b062e34d 100644 --- a/verilog/preprocessor/verilog_preprocess.h +++ b/verilog/preprocessor/verilog_preprocess.h @@ -118,8 +118,7 @@ class VerilogPreprocess { static std::unique_ptr ParseMacroParameter( TokenStreamView::const_iterator*, MacroParameterInfo*); - std::unique_ptr RegisterMacroDefinition( - const MacroDefinition&); + void RegisterMacroDefinition(const MacroDefinition&); // Results of preprocessing VerilogPreprocessData preprocess_data_; diff --git a/verilog/preprocessor/verilog_preprocess_test.cc b/verilog/preprocessor/verilog_preprocess_test.cc index 65aba1de8..adde46f2a 100644 --- a/verilog/preprocessor/verilog_preprocess_test.cc +++ b/verilog/preprocessor/verilog_preprocess_test.cc @@ -14,6 +14,7 @@ #include "verilog/preprocessor/verilog_preprocess.h" +#include #include #include @@ -24,6 +25,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" #include "verilog/analysis/verilog_analyzer.h" +#include "verilog/parser/verilog_token_enum.h" namespace verilog { namespace { @@ -35,21 +37,21 @@ using verible::container::FindOrNull; class PreprocessorTester { public: explicit PreprocessorTester(const char* text) - : analyzer_(text, "<>"), status_() { - status_ = analyzer_.Analyze(); - } + : analyzer_(text, "<>"), status_(analyzer_.Analyze()) {} const VerilogPreprocessData& PreprocessorData() const { return analyzer_.PreprocessorData(); } + const verible::TextStructureView& Data() const { return analyzer_.Data(); } + const absl::Status& Status() const { return status_; } const VerilogAnalyzer& Analyzer() const { return analyzer_; } private: VerilogAnalyzer analyzer_; - absl::Status status_; + const absl::Status status_; }; struct FailTest { @@ -99,6 +101,13 @@ TEST(VerilogPreprocessTest, InvalidPreprocessorInputs) { } } +#define EXPECT_PARSE_OK() \ + do { \ + EXPECT_TRUE(tester.Status().ok()) << "Unexpected analyzer failure."; \ + EXPECT_TRUE(tester.PreprocessorData().errors.empty()); \ + EXPECT_TRUE(tester.Analyzer().GetRejectedTokens().empty()); \ + } while (false) + // Verify that VerilogPreprocess works without any directives. TEST(VerilogPreprocessTest, WorksWithoutDefinitions) { const char* test_cases[] = { @@ -109,11 +118,10 @@ TEST(VerilogPreprocessTest, WorksWithoutDefinitions) { }; for (const auto& test_case : test_cases) { PreprocessorTester tester(test_case); - EXPECT_TRUE(tester.Status().ok()); + EXPECT_PARSE_OK(); + const auto& definitions = tester.PreprocessorData().macro_definitions; EXPECT_TRUE(definitions.empty()); - EXPECT_TRUE(tester.PreprocessorData().errors.empty()); - EXPECT_TRUE(tester.Analyzer().GetRejectedTokens().empty()); } } @@ -128,9 +136,8 @@ TEST(VerilogPreprocessTest, OneMacroDefinitionNoParamsNoValue) { }; for (const auto& test_case : test_cases) { PreprocessorTester tester(test_case); - EXPECT_TRUE(tester.Status().ok()); - EXPECT_TRUE(tester.PreprocessorData().errors.empty()); - EXPECT_TRUE(tester.Analyzer().GetRejectedTokens().empty()); + EXPECT_PARSE_OK(); + const auto& definitions = tester.PreprocessorData().macro_definitions; EXPECT_THAT(definitions, ElementsAre(Pair("FOOOO", testing::_))); auto macro = FindOrNull(definitions, "FOOOO"); @@ -145,10 +152,9 @@ TEST(VerilogPreprocessTest, OneMacroDefinitionNoParamsSimpleValue) { PreprocessorTester tester( "module foo;\nendmodule\n" "`define FOOOO \"bar\"\n"); + EXPECT_PARSE_OK(); + const auto& definitions = tester.PreprocessorData().macro_definitions; - EXPECT_TRUE(tester.Status().ok()) << "Unexpected analyzer failure."; - EXPECT_TRUE(tester.PreprocessorData().errors.empty()); - EXPECT_TRUE(tester.Analyzer().GetRejectedTokens().empty()); EXPECT_THAT(definitions, ElementsAre(Pair("FOOOO", testing::_))); auto macro = FindOrNull(definitions, "FOOOO"); ASSERT_NE(macro, nullptr); @@ -161,10 +167,9 @@ TEST(VerilogPreprocessTest, OneMacroDefinitionOneParamWithValue) { PreprocessorTester tester( "module foo;\nendmodule\n" "`define FOOOO(x) (x+1)\n"); + EXPECT_PARSE_OK(); + const auto& definitions = tester.PreprocessorData().macro_definitions; - EXPECT_TRUE(tester.Status().ok()) << "Unexpected analyzer failure."; - EXPECT_TRUE(tester.PreprocessorData().errors.empty()); - EXPECT_TRUE(tester.Analyzer().GetRejectedTokens().empty()); EXPECT_THAT(definitions, ElementsAre(Pair("FOOOO", testing::_))); auto macro = FindOrNull(definitions, "FOOOO"); ASSERT_NE(macro, nullptr); @@ -181,10 +186,9 @@ TEST(VerilogPreprocessTest, OneMacroDefinitionOneParamDefaultWithValue) { PreprocessorTester tester( "module foo;\nendmodule\n" "`define FOOOO(x=22) (x+3)\n"); + EXPECT_PARSE_OK(); + const auto& definitions = tester.PreprocessorData().macro_definitions; - EXPECT_TRUE(tester.Status().ok()) << "Unexpected analyzer failure."; - EXPECT_TRUE(tester.PreprocessorData().errors.empty()); - EXPECT_TRUE(tester.Analyzer().GetRejectedTokens().empty()); EXPECT_THAT(definitions, ElementsAre(Pair("FOOOO", testing::_))); auto macro = FindOrNull(definitions, "FOOOO"); ASSERT_NE(macro, nullptr); @@ -202,10 +206,9 @@ TEST(VerilogPreprocessTest, TwoMacroDefinitions) { PreprocessorTester tester( "`define BAAAAR(y, z) (y*z)\n" "`define FOOOO(x=22) (x+3)\n"); + EXPECT_PARSE_OK(); + const auto& definitions = tester.PreprocessorData().macro_definitions; - EXPECT_TRUE(tester.Status().ok()) << "Unexpected analyzer failure."; - EXPECT_TRUE(tester.PreprocessorData().errors.empty()); - EXPECT_TRUE(tester.Analyzer().GetRejectedTokens().empty()); EXPECT_THAT(definitions, ElementsAre(Pair("BAAAAR", testing::_), Pair("FOOOO", testing::_))); { @@ -224,5 +227,42 @@ TEST(VerilogPreprocessTest, TwoMacroDefinitions) { } } +TEST(VerilogPreprocessTest, RedefineMacroWarning) { + PreprocessorTester tester( + "`define FOO 1\n" + "`define FOO 2\n"); + EXPECT_PARSE_OK(); + + const auto& definitions = tester.PreprocessorData().macro_definitions; + EXPECT_EQ(definitions.size(), 1); + + const auto& warnings = tester.PreprocessorData().warnings; + EXPECT_EQ(warnings.size(), 1); + EXPECT_EQ(warnings.begin()->error_message, "Re-defining macro"); +} + +// We might have different modes later, in which we remove the define tokens +// from the stream. Document the current default which registeres all the +// defines, but also does not filter out the define calls. +TEST(VerilogPreprocessTest, DefaultPreprocessorKeepsDefineInStream) { + PreprocessorTester tester( + "`define FOO\n" + "`define BAR(x) (x)\n" + "module x(); endmodule\n"); + EXPECT_PARSE_OK(); + + const auto& definitions = tester.PreprocessorData().macro_definitions; + EXPECT_EQ(definitions.size(), 2); + + // The original `define tokens are still in the stream + const auto& token_stream = tester.Data().GetTokenStreamView(); + const int count_defines = + std::count_if(token_stream.begin(), token_stream.end(), + [](verible::TokenSequence::const_iterator t) { + return t->token_enum() == verilog_tokentype::PP_define; + }); + EXPECT_EQ(count_defines, 2); +} + } // namespace } // namespace verilog