log_help: Refactor help content adding

Content is now added to the `ContentListing` rather than the `PrettyHelp`.
`open_*` methods return the `ContentListing` that was added instead of leaving a hanging continuation.
This allows for (e.g.) options to be added directly to optiongroups, instead of requiring that groups be closed before continuation.
This also means that all `PrettyHelp`s are a listing, with the actual log being called by the default `Pass::help()`; making the mode field redundant.
Added `PrettyHelp::log_help()` which replaces the `PrettyHelp::Mode::LOG` logic.
Added `ContentListing::back()` which just returns the last element of the underlying content vector.
Some of the content tracking was made redundant and removed, in particular `PrettyHelp::_current_listing` and `ContentListing::parent`.

Converted `ContentListing` to a class instead of a struct, adjusting constructors to match.
Added `ContentListing` constructor that accepts a `source_location`.

Update `HelpPass::dump_cmds_json()` for new log_help.

kernel/log_help.cc

@@ -28,11 +28,56 @@ Json ContentListing::to_json() {
 	if (strcmp(source_file, "unknown") != 0) object["source_file"] = source_file;
 	if (source_line != 0) object["source_line"] = source_line;
 	Json::array content_array;
-	for (auto child : content) content_array.push_back(child->to_json());
+	for (auto child : _content) content_array.push_back(child->to_json());
 	object["content"] = content_array;
 	return object;
 }
 
+void ContentListing::usage(const string &usage,
+	const source_location location)
+{
+	log_assert(type.compare("root") == 0);
+	add_content("usage", usage, location);
+}
+
+void ContentListing::option(const string &text, const string &description,
+	const source_location location)
+{
+	auto option = open_option(text);
+	if (description.length())
+		option->add_content("text", description, location);
+}
+
+void ContentListing::codeblock(const string &code, const string &language,
+	const source_location location)
+{
+	add_content("code", code, location);
+}
+
+void ContentListing::paragraph(const string &text,
+	const source_location location)
+{
+	add_content("text", text, location);
+}
+
+ContentListing* ContentListing::open_optiongroup(const string &name,
+	const source_location location)
+{
+	log_assert(type.compare("root") == 0);
+	auto optiongroup = new ContentListing("optiongroup", name, location);
+	add_content(optiongroup);
+	return optiongroup;
+}
+
+ContentListing* ContentListing::open_option(const string &text,
+	const source_location location)
+{
+	log_assert(type.compare("optiongroup") == 0);
+	auto option = new ContentListing("option", text, location);
+	add_content(option);
+	return option;
+}
+
 #define MAX_LINE_LEN 80
 void log_pass_str(const std::string &pass_str, std::string indent_str, bool leading_newline=false) {
 	if (pass_str.empty())
@@ -66,15 +111,10 @@ void log_pass_str(const std::string &pass_str, int indent=0, bool leading_newlin
 
 PrettyHelp *current_help = nullptr;
 
-PrettyHelp::PrettyHelp(Mode mode)
+PrettyHelp::PrettyHelp()
 {
 	_prior = current_help;
-	_mode = mode;
+	_root_listing = ContentListing("root", "");
-	_root_listing = ContentListing({
-		.type = "root"
-	});
-	_root_listing.type = "root";
-	_current_listing = &_root_listing;
 
 	current_help = this;
 }
@@ -91,127 +131,22 @@ PrettyHelp *PrettyHelp::get_current()
 	return current_help;
 }
 
-void PrettyHelp::usage(const string &usage,
+void PrettyHelp::log_help()
-	const source_location location)
 {
-	switch (_mode)
+	for (auto content : _root_listing.get_content()) {
-	{
+		if (content->type.compare("usage") == 0) {
-	case LOG:
+			log_pass_str(content->body, 1, true);
-		log_pass_str(usage, _current_indent+1, true);
+		} else if (content->type.compare("optiongroup") == 0) {
-		log("\n");
+			for (auto option : content->get_content()) {
-		break;
+				log_pass_str(option->body, 1);
-	case LISTING:
+				for (auto text : option->get_content()) {
-		add_content("usage", usage, location);
+					log_pass_str(text->body, 2);
-		break;
+					log("\n");
-	default:
+				}
-		log_abort();
+			}
-	}
+		} else {
-}
+			log_pass_str(content->body, 0, true);
-
-void PrettyHelp::option(const string &text, const string &description,
-	const source_location location)
-{
-	open_option(text);
-	if (description.length()) {
-		switch (_mode)
-		{
-		case LOG:
-			log_pass_str(description, _current_indent);
 			log("\n");
-			break;
-		case LISTING:
-			add_content("text", description, location);
-			break;
-		default:
-			log_abort();
 		}
 	}
-	close(1);
-}
-
-void PrettyHelp::codeblock(const string &code, const string &language,
-	const source_location location)
-{
-	switch (_mode)
-	{
-	case LOG:
-		log("%s\n", code.c_str());
-		break;
-	case LISTING:
-		add_content("code", code, location);
-		break;
-	default:
-		log_abort();
-	}
-}
-
-void PrettyHelp::paragraph(const string &text,
-	const source_location location)
-{
-	switch (_mode)
-	{
-	case LOG:
-		log_pass_str(text, _current_indent);
-		log("\n");
-		break;
-	case LISTING:
-		add_content("text", text, location);
-		break;
-	default:
-		log_abort();
-	}
-}
-
-void PrettyHelp::open_optiongroup(const string &name,
-	const source_location location)
-{
-	switch (_mode)
-	{
-	case LOG:
-		break;
-	case LISTING:
-		push_content("optiongroup", name, location);
-		break;
-	default:
-		log_abort();
-	}
-	_current_indent += 1;
-}
-
-void PrettyHelp::open_option(const string &text,
-	const source_location location)
-{
-	switch (_mode)
-	{
-	case LOG:
-		log_pass_str(text, _current_indent);
-		break;
-	case LISTING:
-		push_content("option", text, location);
-		break;
-	default:
-		log_abort();
-	}
-	_current_indent += 1;
-}
-
-void PrettyHelp::close(int levels)
-{
-	
-	switch (_mode)
-	{
-	case LOG:
-		_current_indent -= levels;
-		log_assert(_current_indent >= 0);
-		break;
-	case LISTING:
-		for (int i=0; i<levels; i++) {
-			_current_indent--;
-			log_assert(_current_indent >= 0);
-			pop_content();
-		}
-		break;
-	default:
-		log_abort();
-	}
 }

kernel/log_help.h

@@ -25,73 +25,41 @@
 
 YOSYS_NAMESPACE_BEGIN
 
-struct ContentListing {
+class ContentListing {
-	string type = "root";
+	vector<ContentListing *> _content;
-	string body = "";
+public:
-	const char* source_file = "unknown";
+	string type;
-	int source_line = 0;
+	string body;
-	vector<ContentListing *> content = {};
+	const char* source_file;
-	ContentListing *parent = nullptr;
+	int source_line;
+
+	ContentListing(
+		string type = "root", string body = "",
+		const char* source_file = "unknown", int source_line = 0
+	) : type(type), body(body), source_file(source_file), source_line(source_line) {
+		_content = {};
+	}
+
+	ContentListing(string type, string body, source_location location) :
+		ContentListing(type, body, location.file_name(), location.line()) { }
 
 	void add_content(ContentListing *new_content) {
-		new_content->parent = this;
+		_content.push_back(new_content);
-		content.push_back(new_content);
 	}
 
 	void add_content(string type, string body, source_location location) {
-		auto new_content = new ContentListing();
+		auto new_content = new ContentListing(type, body, location);
-		new_content->type = type;
-		new_content->body = body;
-		new_content->source_file = location.file_name();
-		new_content->source_line = location.line();
 		add_content(new_content);
 	}
 
-	Json to_json();
+	bool has_content() { return _content.size() != 0; }
-};
-
-class PrettyHelp
-{
-public:
-	string group = "unknown";
-	enum Mode {
-		LOG,
-		LISTING,
-	};
-
-private:
-	PrettyHelp *_prior;
-	Mode _mode;
-
-	int _current_indent = 0;
-	ContentListing _root_listing;
-	ContentListing *_current_listing;
-
-	void add_content(string type, string body, source_location location) {
-		_current_listing->add_content(type, body, location);
-	}
-	void push_content(string type, string body, source_location location) {
-		add_content(type, body, location);
-		_current_listing = _current_listing->content.back();
-	}
-	void pop_content() {
-		_current_listing = _current_listing->parent;
-		log_assert(_current_listing != nullptr);
-	}
-public:
-	PrettyHelp(Mode mode = LOG);
-	~PrettyHelp();
-
-	static PrettyHelp *get_current();
-
-	bool has_content() { return _root_listing.content.size();}
 	const vector<ContentListing *> get_content() {
-		const vector<ContentListing *> content = _root_listing.content;
+		const vector<ContentListing *> content = _content;
 		return content;
 	}
-
+	ContentListing* back() {
-	void set_group(const string g) { group = g; }
+		return _content.back();
-	bool has_group() { return group.compare("unknown") != 0; }
+	}
 
 	void usage(
 		const string &usage,
@@ -112,15 +80,45 @@ public:
 		const source_location location = source_location::current()
 	);
 
-	void open_optiongroup(
+	ContentListing* open_optiongroup(
 		const string &name = "",
 		const source_location location = source_location::current()
 	);
-	void open_option(
+	ContentListing* open_option(
 		const string &text,
 		const source_location location = source_location::current()
 	);
-	void close(int levels = 1);
+
+	Json to_json();
+};
+
+class PrettyHelp
+{
+public:
+	string group = "unknown";
+
+private:
+	PrettyHelp *_prior;
+	ContentListing _root_listing;
+
+public:
+	PrettyHelp();
+	~PrettyHelp();
+
+	static PrettyHelp *get_current();
+
+	bool has_content() { return _root_listing.has_content(); }
+	const vector<ContentListing *> get_content() {
+		return _root_listing.get_content();
+	}
+	ContentListing* get_root() {
+		return &_root_listing;
+	}
+
+	void set_group(const string g) { group = g; }
+	bool has_group() { return group.compare("unknown") != 0; }
+
+	void log_help();
 };
 
 YOSYS_NAMESPACE_END

kernel/register.cc

@@ -118,7 +118,10 @@ void Pass::post_execute(Pass::pre_post_exec_state_t state)
 
 void Pass::help()
 {
-	if (!formatted_help()) {
+	auto prettyHelp = PrettyHelp();
+	if (formatted_help()) {
+		prettyHelp.log_help();
+	} else {
 		log("\n");
 		log("No help message for command `%s'.\n", pass_name.c_str());
 		log("\n");
@@ -826,7 +829,7 @@ struct HelpPass : public Pass {
 			auto title = pass->short_help;
 			auto experimental_flag = pass->experimental_flag;
 
-			auto cmd_help = PrettyHelp(PrettyHelp::Mode::LISTING);
+			auto cmd_help = PrettyHelp();
 			auto has_pretty_help = pass->formatted_help();
 
 			if (!has_pretty_help) {
@@ -839,6 +842,8 @@ struct HelpPass : public Pass {
 
 				source_location null_source;
 				string current_buffer = "";
+				auto root_listing = cmd_help.get_root();
+				auto current_listing = root_listing;
 
 				// dump command help
 				std::ostringstream buf;
@@ -861,7 +866,8 @@ struct HelpPass : public Pass {
 						switch (current_state)
 						{
 						case PUState_signature:
-							cmd_help.usage(current_buffer, null_source);
+							root_listing->usage(current_buffer, null_source);
+							current_listing = root_listing;
 							current_state = PUState_none;
 							current_buffer = "";
 							break;
@@ -888,16 +894,16 @@ struct HelpPass : public Pass {
 
 					if (IsSignature && first_pos <= 4 && (blank_lines >= 2 || current_state == PUState_signature)) {
 						if (current_state == PUState_options || current_state == PUState_optionbody) {
-							cmd_help.codeblock(current_buffer, "none", null_source);
+							current_listing->codeblock(current_buffer, "none", null_source);
 							current_buffer = "";
-							cmd_help.close(2);
 						} else if (current_state == PUState_signature) {
-							cmd_help.usage(current_buffer, null_source);
+							root_listing->usage(current_buffer, null_source);
 							current_buffer = "";
 						} else if (current_state == PUState_none && !current_buffer.empty()) {
-							cmd_help.codeblock(current_buffer, "none", null_source);
+							current_listing->codeblock(current_buffer, "none", null_source);
 							current_buffer = "";
 						}
+						current_listing = root_listing;
 						current_state = PUState_signature;
 						def_strip_count = first_pos;
 						catch_verific = false;
@@ -905,15 +911,15 @@ struct HelpPass : public Pass {
 						def_strip_count = first_pos;
 						if (current_state == PUState_optionbody) {
 							if (!current_buffer.empty()) {
-								cmd_help.codeblock(current_buffer, "none", null_source);
+								current_listing->codeblock(current_buffer, "none", null_source);
 								current_buffer = "";
 							}
 							if (IsIndent) {
 								current_state = PUState_options;
-								cmd_help.close(1);
+								current_listing = root_listing->back();
 							} else {
 								current_state = PUState_none;
-								cmd_help.close(2);
+								current_listing = root_listing;
 							}
 						} else {
 							current_state = PUState_none;
@@ -922,16 +928,16 @@ struct HelpPass : public Pass {
 
 					if (IsDefinition && !catch_verific && current_state != PUState_signature) {
 						if (!current_buffer.empty()) {
-							cmd_help.codeblock(current_buffer, "none", null_source);
+							current_listing->codeblock(current_buffer, "none", null_source);
 							current_buffer = "";
 						}
 						if (current_state == PUState_options || current_state == PUState_optionbody) {
-							cmd_help.close(1);
+							current_listing = root_listing->back();
 						}  else {
-							cmd_help.open_optiongroup("", null_source);
+							current_listing = root_listing->open_optiongroup("", null_source);
 						}
 						current_state = PUState_options;
-						cmd_help.open_option(stripped_line, null_source);
+						current_listing = current_listing->open_option(stripped_line, null_source);
 						def_strip_count = first_pos;
 					} else {
 						if (current_state == PUState_options) {
@@ -952,7 +958,7 @@ struct HelpPass : public Pass {
 				}
 
 				if (!current_buffer.empty()) {
-					cmd_help.codeblock(current_buffer, "none", null_source);
+					current_listing->codeblock(current_buffer, "none", null_source);
 					current_buffer = "";
 				}
 			}

passes/cmds/chformal.cc

@@ -76,57 +76,57 @@ struct ChformalPass : public Pass {
 	bool formatted_help() override {
 		auto *help = PrettyHelp::get_current();
 		help->set_group("formal");
-		help->usage("chformal [types] [mode] [options] [selection]");
+
-		help->paragraph(
+		auto content_root = help->get_root();
+
+		content_root->usage("chformal [types] [mode] [options] [selection]");
+		content_root->paragraph(
 			"Make changes to the formal constraints of the design. The [types] options "
 			"the type of constraint to operate on. If none of the following options are "
 			"given, the command will operate on all constraint types:"
 		);
 
-		help->open_optiongroup("[types]");
+		auto types_group = content_root->open_optiongroup("[types]");
-		help->option("-assert", "`$assert` cells, representing ``assert(...)`` constraints");
+		types_group->option("-assert", "`$assert` cells, representing ``assert(...)`` constraints");
-		help->option("-assume", "`$assume` cells, representing ``assume(...)`` constraints");
+		types_group->option("-assume", "`$assume` cells, representing ``assume(...)`` constraints");
-		help->option("-live", "`$live` cells, representing ``assert(s_eventually ...)``");
+		types_group->option("-live", "`$live` cells, representing ``assert(s_eventually ...)``");
-		help->option("-fair", "`$fair` cells, representing ``assume(s_eventually ...)``");
+		types_group->option("-fair", "`$fair` cells, representing ``assume(s_eventually ...)``");
-		help->option("-cover", "`$cover` cells, representing ``cover()`` statements");
+		types_group->option("-cover", "`$cover` cells, representing ``cover()`` statements");
-		help->paragraph(
+		types_group->paragraph(
 			"Additionally chformal will operate on `$check` cells corresponding to the "
 			"selected constraint types."
 		);
-		help->close();
 
-		help->paragraph("Exactly one of the following modes must be specified:");
+		content_root->paragraph("Exactly one of the following modes must be specified:");
 
-		help->open_optiongroup("[mode]");
+		auto modes_group = content_root->open_optiongroup("[mode]");
-		help->option("-remove", "remove the cells and thus constraints from the design");
+		modes_group->option("-remove", "remove the cells and thus constraints from the design");
-		help->option("-early",
+		modes_group->option("-early",
 			"bypass FFs that only delay the activation of a constraint. When inputs "
 			"of the bypassed FFs do not remain stable between clock edges, this may "
 			"result in unexpected behavior."
 		);
-		help->option("-delay <N>", "delay activation of the constraint by <N> clock cycles");
+		modes_group->option("-delay <N>", "delay activation of the constraint by <N> clock cycles");
-		help->option("-skip <N>", "ignore activation of the constraint in the first <N> clock cycles");
+		modes_group->option("-skip <N>", "ignore activation of the constraint in the first <N> clock cycles");
-		help->open_option("-coverenable");
+		auto cover_option = modes_group->open_option("-coverenable");
-		help->paragraph(
+		cover_option->paragraph(
 			"add cover statements for the enable signals of the constraints"
 		);
 #ifdef YOSYS_ENABLE_VERIFIC
-		help->paragraph(
+		cover_option->paragraph(
 			"Note: For the Verific frontend it is currently not guaranteed that a "
 			"reachable SVA statement corresponds to an active enable signal."
 		);
 #endif
-		help->close();
+		modes_group->option("-assert2assume");
-		help->option("-assert2assume");
+		modes_group->option("-assume2assert");
-		help->option("-assume2assert");
+		modes_group->option("-live2fair");
-		help->option("-live2fair");
+		modes_group->option("-fair2live", "change the roles of cells as indicated. these options can be combined");
-		help->option("-fair2live", "change the roles of cells as indicated. these options can be combined");
+		modes_group->option("-lower",
-		help->option("-lower",
 			"convert each $check cell into an $assert, $assume, $live, $fair or "
 			"$cover cell. If the $check cell contains a message, also produce a "
 			"$print cell."
 		);
-		help->close();
 		return true;
 	}
 	void execute(std::vector<std::string> args, RTLIL::Design *design) override