Merge origin/master into fix/2544 Resolve formatter_test.cc conflict by keeping MacroBeforeCloseParenFormatEquivalent alongside EndElseIfWithEOLCommentConverges from merged #2541.
diff --git a/verible/verilog/formatting/formatter_test.cc b/verible/verilog/formatting/formatter_test.cc index 9c59626..5501c58 100644 --- a/verible/verilog/formatting/formatter_test.cc +++ b/verible/verilog/formatting/formatter_test.cc
@@ -19398,6 +19398,78 @@ EXPECT_THAT(stream.str(), testing::HasSubstr("`TOKEN_BYTE")); } +// Regression for https://github.com/chipsalliance/verible/issues/2540: +// Trailing EOL comment after `end` before `else if` must not change whether +// the else-if assignment stays on one line across re-format (convergence). +TEST(FormatterEndToEndTest, EndElseIfWithEOLCommentConverges) { + static constexpr FormatterTestCase kTestCases[] = { + {// Comment on its own line between end and else if + "module m;\n" + " always_comb begin\n" + " case (state)\n" + " STATE_A: begin\n" + " if (cond_aaaa) next_state_value = STATE_B;\n" + " else if (cond_bbbb) begin\n" + " next_state_value = STATE_B;\n" + " end\n" + " // xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n" + " else if (cond_cccc) next_state_value = STATE_C;\n" + " end\n" + " endcase\n" + " end\n" + "endmodule\n", + "module m;\n" + " always_comb begin\n" + " case (state)\n" + " STATE_A: begin\n" + " if (cond_aaaa) next_state_value = STATE_B;\n" + " else if (cond_bbbb) begin\n" + " next_state_value = STATE_B;\n" + " end // xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n" + " else if (cond_cccc) next_state_value = STATE_C;\n" + " end\n" + " endcase\n" + " end\n" + "endmodule\n"}, + {// Same construct with comment already on the end line + "module m;\n" + " always_comb begin\n" + " case (state)\n" + " STATE_A: begin\n" + " if (cond_aaaa) next_state_value = STATE_B;\n" + " else if (cond_bbbb) begin\n" + " next_state_value = STATE_B;\n" + " end // xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n" + " else if (cond_cccc) next_state_value = STATE_C;\n" + " end\n" + " endcase\n" + " end\n" + "endmodule\n", + "module m;\n" + " always_comb begin\n" + " case (state)\n" + " STATE_A: begin\n" + " if (cond_aaaa) next_state_value = STATE_B;\n" + " else if (cond_bbbb) begin\n" + " next_state_value = STATE_B;\n" + " end // xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n" + " else if (cond_cccc) next_state_value = STATE_C;\n" + " end\n" + " endcase\n" + " end\n" + "endmodule\n"}, + }; + FormatStyle style; // default column_limit (100) + for (const auto &test_case : kTestCases) { + VLOG(1) << "code-to-format:\n" << test_case.input << "<EOF>"; + std::ostringstream stream; + const auto status = + FormatVerilog(test_case.input, "<filename>", style, stream); + EXPECT_OK(status) << status.message(); + EXPECT_EQ(stream.str(), test_case.expected) << "code:\n" << test_case.input; + } +} + // Verify kAlign behavior for body-level param/localparam declarations // in module and package bodies. TEST(FormatterEndToEndTest, ParamDeclarationAlignmentBasics) {
diff --git a/verible/verilog/formatting/tree-unwrapper.cc b/verible/verilog/formatting/tree-unwrapper.cc index 9664c98..a6601ac 100644 --- a/verible/verilog/formatting/tree-unwrapper.cc +++ b/verible/verilog/formatting/tree-unwrapper.cc
@@ -1819,6 +1819,14 @@ } } +// True if any token in this leaf partition is an EOL comment. +static bool PartitionContainsEOLComment(const TokenPartitionTree &partition) { + for (const auto &token : partition.Value().TokensRange()) { + if (token.TokenEnum() == verilog_tokentype::TK_EOL_COMMENT) return true; + } + return false; +} + static void PushEndIntoElsePartition(TokenPartitionTree *partition_ptr) { // Then combine 'end' with the following 'else' ... // Do not flatten, so that if- and else- clauses can make formatting @@ -1826,6 +1834,15 @@ auto &partition = *partition_ptr; auto &if_clause_partition = partition.Children().front(); auto *end_partition = &RightmostDescendant(if_clause_partition); + // When 'end' carries a trailing EOL comment, 'else' must start on the next + // line (see token annotator: comment before else => MustWrap). Merging + // end+comment into the else-if header makes fit-else-expand treat the + // header as wider than the eventual formatted line, which wraps the + // else-if body on re-format and fails convergence (GitHub issue 2540). + if (PartitionContainsEOLComment(*end_partition)) { + VLOG(4) << "end has EOL comment, skip merge into else"; + return; + } auto *end_parent = verible::MergeLeafIntoNextLeaf(end_partition); // if moving leaf results in any singleton partitions, hoist. if (end_parent != nullptr) {
diff --git a/verible/verilog/tools/kythe/README.md b/verible/verilog/tools/kythe/README.md index b44aaf9..610f755 100644 --- a/verible/verilog/tools/kythe/README.md +++ b/verible/verilog/tools/kythe/README.md
@@ -30,4 +30,6 @@ File search will stop at the the first found among the listed directories. e.g --include_dir_paths directory1,directory2 if "A.sv" exists in both "directory1" and "directory2" the one in "directory1" is the one we will use) + --output_path (Path of the file to write the extracted Kythe facts to. + Writes to stdout if empty or "-"); default: ""; ```
diff --git a/verible/verilog/tools/kythe/kythe-proto-output.cc b/verible/verilog/tools/kythe/kythe-proto-output.cc index 0117329..76e5952 100644 --- a/verible/verilog/tools/kythe/kythe-proto-output.cc +++ b/verible/verilog/tools/kythe/kythe-proto-output.cc
@@ -14,9 +14,12 @@ #include "verible/verilog/tools/kythe/kythe-proto-output.h" +#include <memory> +#include <ostream> #include <string> #include "google/protobuf/io/coded_stream.h" +#include "google/protobuf/io/zero_copy_stream.h" #include "google/protobuf/io/zero_copy_stream_impl.h" #include "third_party/proto/kythe/storage.pb.h" #include "verible/verilog/tools/kythe/kythe-facts.h" @@ -26,7 +29,8 @@ namespace { using ::google::protobuf::io::CodedOutputStream; -using ::google::protobuf::io::FileOutputStream; +using ::google::protobuf::io::OstreamOutputStream; +using ::google::protobuf::io::ZeroCopyOutputStream; using ::kythe::proto::Entry; // Returns the VName representation in Kythe's storage proto format. @@ -60,7 +64,7 @@ } // Output entry to the stream. -void OutputProto(const Entry &entry, FileOutputStream *stream) { +void OutputProto(const Entry &entry, ZeroCopyOutputStream *stream) { CodedOutputStream coded_stream(stream); coded_stream.WriteVarint32(entry.ByteSizeLong()); entry.SerializeToCodedStream(&coded_stream); @@ -68,14 +72,17 @@ } // namespace -KytheProtoOutput::KytheProtoOutput(int fd) : out_(fd) {} -KytheProtoOutput::~KytheProtoOutput() { out_.Close(); } +KytheProtoOutput::KytheProtoOutput(std::ostream &output_stream) + : out_(std::make_unique<OstreamOutputStream>(&output_stream)) {} + +// OstreamOutputStream flushes its remaining bytes when destroyed. +KytheProtoOutput::~KytheProtoOutput() = default; void KytheProtoOutput::Emit(const Fact &fact) { - OutputProto(ConvertFactToEntry(fact), &out_); + OutputProto(ConvertFactToEntry(fact), out_.get()); } void KytheProtoOutput::Emit(const Edge &edge) { - OutputProto(ConvertEdgeToEntry(edge), &out_); + OutputProto(ConvertEdgeToEntry(edge), out_.get()); } } // namespace kythe
diff --git a/verible/verilog/tools/kythe/kythe-proto-output.h b/verible/verilog/tools/kythe/kythe-proto-output.h index ba873cd..2ffa58e 100644 --- a/verible/verilog/tools/kythe/kythe-proto-output.h +++ b/verible/verilog/tools/kythe/kythe-proto-output.h
@@ -15,7 +15,10 @@ #ifndef VERIBLE_VERILOG_TOOLS_KYTHE_KYTHE_PROTO_OUTPUT_H_ #define VERIBLE_VERILOG_TOOLS_KYTHE_KYTHE_PROTO_OUTPUT_H_ -#include "google/protobuf/io/zero_copy_stream_impl.h" +#include <memory> +#include <ostream> + +#include "google/protobuf/io/zero_copy_stream.h" #include "verible/verilog/tools/kythe/kythe-facts-extractor.h" #include "verible/verilog/tools/kythe/kythe-facts.h" @@ -24,7 +27,10 @@ class KytheProtoOutput final : public KytheOutput { public: - explicit KytheProtoOutput(int output_fd); + // Writes to an already-open stream. The stream must outlive this object and + // must be opened in binary mode, as the proto entries are not text. + explicit KytheProtoOutput(std::ostream &output_stream); + ~KytheProtoOutput() final; // Output Kythe facts from the indexing data in proto format. @@ -32,7 +38,7 @@ void Emit(const Edge &edge) final; private: - ::google::protobuf::io::FileOutputStream out_; + std::unique_ptr<::google::protobuf::io::ZeroCopyOutputStream> out_; }; } // namespace kythe
diff --git a/verible/verilog/tools/kythe/verilog-kythe-extractor.cc b/verible/verilog/tools/kythe/verilog-kythe-extractor.cc index df43e52..321db49 100644 --- a/verible/verilog/tools/kythe/verilog-kythe-extractor.cc +++ b/verible/verilog/tools/kythe/verilog-kythe-extractor.cc
@@ -12,7 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include <fstream> +#include <ios> #include <iostream> +#include <memory> +#include <ostream> #include <sstream> #include <string> #include <string_view> @@ -33,11 +37,9 @@ #include "verible/verilog/tools/kythe/kythe-facts.h" #include "verible/verilog/tools/kythe/kythe-proto-output.h" -#ifndef _WIN32 -#include <unistd.h> // for STDOUT_FILENO -#else -#include <stdio.h> -#define STDOUT_FILENO _fileno(stdout) +#ifdef _WIN32 +#include <fcntl.h> +#include <io.h> #endif // for --print_kythe_facts flag @@ -109,14 +111,18 @@ ABSL_FLAG(std::string, verilog_project_name, "", "Verilog project name to use as Kythe corpus. Optional"); +ABSL_FLAG(std::string, output_path, "", + R"(Path of the file to write the extracted Kythe facts to. +Writes to stdout if empty or "-".)"); + namespace verilog { namespace kythe { -// Prints Kythe facts in proto format to stdout. +// Prints Kythe facts in proto format to the given stream. static void PrintKytheFactsProtoEntries( const IndexingFactNode &file_list_facts_tree, const VerilogProject &project, - int fd) { - KytheProtoOutput proto_output(fd); + std::ostream &stream) { + KytheProtoOutput proto_output(stream); StreamKytheFactsEntries(&proto_output, file_list_facts_tree, project); } @@ -134,7 +140,7 @@ static std::vector<absl::Status> ExtractTranslationUnits( std::string_view file_list_path, VerilogProject *project, - const std::vector<std::string> &file_names) { + const std::vector<std::string> &file_names, std::ostream &output_stream) { std::vector<absl::Status> errors; const verilog::kythe::IndexingFactNode file_list_facts_tree( verilog::kythe::ExtractFiles(file_list_path, project, file_names, @@ -149,17 +155,17 @@ // check how to output kythe facts. switch (absl::GetFlag(FLAGS_print_kythe_facts)) { case PrintMode::kJSON: - std::cout << KytheFactsPrinter(file_list_facts_tree, *project) - << std::endl; + output_stream << KytheFactsPrinter(file_list_facts_tree, *project) + << std::endl; break; case PrintMode::kJSONDebug: - std::cout << KytheFactsPrinter(file_list_facts_tree, *project, - /*debug=*/true) - << std::endl; + output_stream << KytheFactsPrinter(file_list_facts_tree, *project, + /*debug=*/true) + << std::endl; break; case PrintMode::kProto: PrintKytheFactsProtoEntries(file_list_facts_tree, *project, - STDOUT_FILENO); + output_stream); break; case PrintMode::kNone: KytheFactsNullPrinter(file_list_facts_tree, *project); @@ -173,6 +179,13 @@ } // namespace verilog int main(int argc, char **argv) { +#ifdef _WIN32 + // Windows messes with newlines by default. Fix this here, so that stdout + // carries the same bytes as --output_path, and so that the proto entries + // stay binary. + _setmode(_fileno(stdout), _O_BINARY); +#endif + const auto usage = absl::StrCat("usage: ", argv[0], " [options] --file_list_path FILE\n", R"( Extracts kythe indexing facts from the given SystemVerilog source files. @@ -211,9 +224,24 @@ absl::GetFlag(FLAGS_verilog_project_name), /*provide_lookup_file_origin=*/false); + // Send the facts to a file when asked to, otherwise to stdout. Both go out + // in binary mode, because --print_kythe_facts=proto is not text. + const std::string output_path = absl::GetFlag(FLAGS_output_path); + std::unique_ptr<std::ofstream> file_closer; + std::ostream *output_stream = &std::cout; + if (!output_path.empty() && output_path != "-") { + file_closer = std::make_unique<std::ofstream>( + output_path, std::ios::out | std::ios::binary); + if (!file_closer->good()) { + LOG(ERROR) << "Failed to create/open output file: " << output_path; + return 1; + } + output_stream = file_closer.get(); + } + const std::vector<absl::Status> errors( verilog::kythe::ExtractTranslationUnits(file_list_path, &project, - file_paths)); + file_paths, *output_stream)); if (!errors.empty()) { LOG(ERROR) << "Encountered some issues while indexing files (could result " "in missing indexing data):"
diff --git a/verible/verilog/tools/kythe/verilog_kythe_extractor_test.sh b/verible/verilog/tools/kythe/verilog_kythe_extractor_test.sh index bf3bd7c..1e7bcc9 100755 --- a/verible/verilog/tools/kythe/verilog_kythe_extractor_test.sh +++ b/verible/verilog/tools/kythe/verilog_kythe_extractor_test.sh
@@ -160,4 +160,90 @@ } ################################################################################ +echo "=== Write facts to --output_path instead of stdout." + +cat > "$MY_INPUT_FILE" <<EOF +localparam int fooo = 1; +localparam int barr = fooo; +EOF + +echo "myinput.txt" > "${TEST_TMPDIR}/file_list" + +# Every print mode has to honor --output_path, and has to write the same bytes +# it would have written to stdout. +for mode in json json_debug proto ; do + "$extractor" \ + --file_list_path "${TEST_TMPDIR}/file_list" \ + --file_list_root "$(dirname "$MY_INPUT_FILE")" \ + --print_kythe_facts="$mode" \ + --output_path "$MY_OUTPUT_FILE" 2>/dev/null + + status="$?" + [[ $status == 0 ]] || { + echo "Expected exit code 0 for --print_kythe_facts=$mode, but got $status" + exit 1 + } + + [[ -s "$MY_OUTPUT_FILE" ]] || { + echo "Expected --output_path file to be non-empty for mode $mode." + exit 1 + } + + "$extractor" \ + --file_list_path "${TEST_TMPDIR}/file_list" \ + --file_list_root "$(dirname "$MY_INPUT_FILE")" \ + --print_kythe_facts="$mode" \ + > "$MY_EXPECT_FILE" 2>/dev/null + + cmp "$MY_OUTPUT_FILE" "$MY_EXPECT_FILE" || { + echo "--output_path output differs from stdout output for mode $mode." + exit 1 + } +done + +################################################################################ +echo "=== '--output_path -' writes to stdout." + +"$extractor" \ + --file_list_path "${TEST_TMPDIR}/file_list" \ + --file_list_root "$(dirname "$MY_INPUT_FILE")" \ + --print_kythe_facts=json \ + --output_path - \ + > "$MY_OUTPUT_FILE" 2>/dev/null + +status="$?" +[[ $status == 0 ]] || { + echo "Expected exit code 0, but got $status" + exit 1 +} + +grep -q "signature" "$MY_OUTPUT_FILE" || { + echo "Expected \"signature\" in $MY_OUTPUT_FILE but didn't find it. Got:" + cat "$MY_OUTPUT_FILE" + exit 1 +} + +################################################################################ +echo "=== Expect failure on unwritable --output_path." + +"$extractor" \ + --file_list_path "${TEST_TMPDIR}/file_list" \ + --file_list_root "$(dirname "$MY_INPUT_FILE")" \ + --print_kythe_facts=json \ + --output_path "${TEST_TMPDIR}/nonexistent-dir/out.json" \ + > "$MY_OUTPUT_FILE" 2>&1 + +status="$?" +[[ $status == 1 ]] || { + echo "Expected exit code 1, but got $status" + exit 1 +} + +grep -q "Failed to create/open output file" "$MY_OUTPUT_FILE" || { + echo "Expected \"Failed to create/open output file\" in $MY_OUTPUT_FILE but didn't find it. Got:" + cat "$MY_OUTPUT_FILE" + exit 1 +} + +################################################################################ echo "PASS"