emu/ioport.cpp: Improved validation of DIP switch locations.

* Treat an empty switch name as an error.
* Treat a non-positive switch number as an error.
* Also allocate fewer temporary strings.

Author: cuavas

src/emu/ioport.cpp

@@ -32,6 +32,7 @@
 #include <algorithm>
 #include <cctype>
 #include <ctime>
+#include <sstream>
 
 
 namespace {
@@ -420,16 +421,13 @@ u8 const inp_header::MAGIC[inp_header::OFFS_BASETIME - inp_header::OFFS_MAGIC] =
 //  to the current list
 //-------------------------------------------------
 
-void ioport_list::append(device_t &device, std::string &errorbuf)
+void ioport_list::append(device_t &device, std::ostream &errorbuf)
 {
 	// no constructor, no list
 	ioport_constructor constructor = device.input_ports();
-	if (constructor == nullptr)
+	if (!constructor)
 		return;
 
-	// reset error buffer
-	errorbuf.clear();
-
 	// detokenize into the list
 	(*constructor)(device, *this, errorbuf);
 
@@ -692,7 +690,7 @@ ioport_setting::ioport_setting(ioport_field &field, ioport_value _value, const c
 //  ioport_diplocation - constructor
 //-------------------------------------------------
 
-ioport_diplocation::ioport_diplocation(const char *name, u8 swnum, bool invert) :
+ioport_diplocation::ioport_diplocation(std::string_view name, u8 swnum, bool invert) :
 	m_name(name),
 	m_number(swnum),
 	m_invert(invert)
@@ -1359,7 +1357,7 @@ float ioport_field::crosshair_read() const
 //  descriptions
 //-------------------------------------------------
 
-void ioport_field::expand_diplocation(const char *location, std::string &errorbuf)
+void ioport_field::expand_diplocation(const char *location, std::ostream &errorbuf)
 {
 	// if nothing present, bail
 	if (!location)
@@ -1368,70 +1366,76 @@ void ioport_field::expand_diplocation(const char *location, std::string &errorbu
 	m_diploclist.clear();
 
 	// parse the string
-	std::string name; // Don't move this variable inside the loop, lastname's lifetime depends on it being outside
-	const char *lastname = nullptr;
+	std::string_view lastname;
 	const char *curentry = location;
 	int entries = 0;
-	while (*curentry != 0)
+	while (*curentry)
 	{
 		// find the end of this entry
 		const char *comma = strchr(curentry, ',');
-		if (comma == nullptr)
+		if (!comma)
 			comma = curentry + strlen(curentry);
 
 		// extract it to tempbuf
-		std::string tempstr(curentry, comma - curentry);
+		std::string_view tempstr(curentry, comma - curentry);
 
 		// first extract the switch name if present
-		const char *number = tempstr.c_str();
-		const char *colon = strchr(tempstr.c_str(), ':');
+		std::string_view::size_type number = 0;
+		std::string_view::size_type const colon = tempstr.find(':');
 
-		if (colon != nullptr)
+		std::string_view name;
+		if (colon != std::string_view::npos)
 		{
 			// allocate and copy the name if it is present
-			lastname = name.assign(number, colon - number).c_str();
+			lastname = tempstr.substr(0, colon);
 			number = colon + 1;
+			if (lastname.empty())
+			{
+				util::stream_format(errorbuf, "Switch location '%s' has empty switch name!\n", location);
+				lastname = "UNK";
+			}
+			name = lastname;
 		}
 		else
 		{
 			// otherwise, just copy the last name
-			if (lastname == nullptr)
+			if (lastname.empty())
 			{
-				errorbuf.append(string_format("Switch location '%s' missing switch name!\n", location));
-				lastname = (char *)"UNK";
+				util::stream_format(errorbuf, "Switch location '%s' missing switch name!\n", location);
+				lastname = "UNK";
 			}
-			name.assign(lastname);
+			name = lastname;
 		}
 
 		// if the number is preceded by a '!' it's active high
-		bool invert = false;
-		if (*number == '!')
-		{
-			invert = true;
-			number++;
-		}
+		bool const invert = tempstr[number] == '!';
+		if (invert)
+			++number;
 
 		// now scan the switch number
 		int swnum = -1;
-		if (sscanf(number, "%d", &swnum) != 1)
-			errorbuf.append(string_format("Switch location '%s' has invalid format!\n", location));
+		if (sscanf(&tempstr[number], "%d", &swnum) != 1)
+			util::stream_format(errorbuf, "Switch location '%s' has invalid format!\n", location);
+		else if (0 >= swnum)
+			util::stream_format(errorbuf, "Switch location '%s' has switch number that is not positive!\n", location);
 
 		// allocate a new entry
-		m_diploclist.emplace_back(name.c_str(), swnum, invert);
+		if (0 < swnum)
+			m_diploclist.emplace_back(name, swnum, invert);
 		entries++;
 
 		// advance to the next item
 		curentry = comma;
-		if (*curentry != 0)
+		if (*curentry)
 			curentry++;
 	}
 
 	// then verify the number of bits in the mask matches
 	int const bits = population_count_32(m_mask);
 	if (bits > entries)
-		errorbuf.append(string_format("Switch location '%s' does not describe enough bits for mask %X\n", location, m_mask));
+		util::stream_format(errorbuf, "Switch location '%s' does not describe enough bits for mask %X\n", location, m_mask);
 	else if (bits < entries)
-		errorbuf.append(string_format("Switch location '%s' describes too many bits for mask %X\n", location, m_mask));
+		util::stream_format(errorbuf, "Switch location '%s' describes too many bits for mask %X\n", location, m_mask);
 }
 
 
@@ -1638,7 +1642,7 @@ void ioport_port::frame_update()
 //  wholly overlapped by other fields
 //-------------------------------------------------
 
-void ioport_port::collapse_fields(std::string &errorbuf)
+void ioport_port::collapse_fields(std::ostream &errorbuf)
 {
 	ioport_value maskbits = 0;
 	int lastmodcount = -1;
@@ -1667,13 +1671,13 @@ void ioport_port::collapse_fields(std::string &errorbuf)
 //  for errors
 //-------------------------------------------------
 
-void ioport_port::insert_field(ioport_field &newfield, ioport_value &disallowedbits, std::string &errorbuf)
+void ioport_port::insert_field(ioport_field &newfield, ioport_value &disallowedbits, std::ostream &errorbuf)
 {
 	// verify against the disallowed bits, but only if we are condition-free
 	if (newfield.condition().none())
 	{
 		if ((newfield.mask() & disallowedbits) != 0)
-			errorbuf.append(string_format("INPUT_TOKEN_FIELD specifies duplicate port bits (port=%s mask=%X)\n", tag(), newfield.mask()));
+			util::stream_format(errorbuf, "INPUT_TOKEN_FIELD specifies duplicate port bits (port=%s mask=%X)\n", tag(), newfield.mask());
 		disallowedbits |= newfield.mask();
 	}
 
@@ -1812,12 +1816,17 @@ time_t ioport_manager::initialize()
 
 	// if we have a token list, proceed
 	device_enumerator iter(machine().root_device());
-	for (device_t &device : iter)
 	{
-		std::string errors;
-		m_portlist.append(device, errors);
-		if (!errors.empty())
-			osd_printf_error("Input port errors:\n%s", errors);
+		std::ostringstream errors;
+		for (device_t &device : iter)
+		{
+			m_portlist.append(device, errors);
+			if (errors.tellp())
+			{
+				osd_printf_error("Input port errors:\n%s", std::move(errors).str());
+				errors.str("");
+			}
+		}
 	}
 
 	// renumber player numbers for controller ports
@@ -3274,7 +3283,7 @@ void ioport_manager::record_port(ioport_port &port)
 //  ioport_configurer - constructor
 //-------------------------------------------------
 
-ioport_configurer::ioport_configurer(device_t &owner, ioport_list &portlist, std::string &errorbuf) :
+ioport_configurer::ioport_configurer(device_t &owner, ioport_list &portlist, std::ostream &errorbuf) :
 	m_owner(owner),
 	m_portlist(portlist),
 	m_errorbuf(errorbuf),

src/emu/ioport.h

@@ -25,9 +25,11 @@
 #include <cstdint>
 #include <cstring>
 #include <ctime>
+#include <iosfwd>
 #include <initializer_list>
 #include <list>
 #include <memory>
+#include <string_view>
 #include <vector>
 
 
@@ -325,7 +327,7 @@ enum
 //**************************************************************************
 
 // constructor function pointer
-typedef void(*ioport_constructor)(device_t &owner, ioport_list &portlist, std::string &errorbuf);
+typedef void(*ioport_constructor)(device_t &owner, ioport_list &portlist, std::ostream &errorbuf);
 
 // I/O port callback function delegates
 typedef device_delegate<ioport_value ()> ioport_field_read_delegate;
@@ -529,7 +531,7 @@ class ioport_diplocation
 {
 public:
 	// construction/destruction
-	ioport_diplocation(const char *name, u8 swnum, bool invert);
+	ioport_diplocation(std::string_view name, u8 swnum, bool invert);
 
 	// getters
 	const char *name() const { return m_name.c_str(); }
@@ -664,7 +666,7 @@ class ioport_field
 	void set_user_settings(const user_settings &settings);
 
 private:
-	void expand_diplocation(const char *location, std::string &errorbuf);
+	void expand_diplocation(const char *location, std::ostream &errorbuf);
 
 	// internal state
 	ioport_field *              m_next;             // pointer to next field in sequence
@@ -744,7 +746,7 @@ class ioport_list : public std::map<std::string, std::unique_ptr<ioport_port>>
 public:
 	ioport_list() { }
 
-	void append(device_t &device, std::string &errorbuf);
+	void append(device_t &device, std::ostream &errorbuf);
 };
 
 
@@ -779,13 +781,13 @@ class ioport_port
 
 	// other operations
 	ioport_field *field(ioport_value mask) const;
-	void collapse_fields(std::string &errorbuf);
+	void collapse_fields(std::ostream &errorbuf);
 	void frame_update();
 	void init_live_state();
 	void update_defvalue(bool flush_defaults);
 
 private:
-	void insert_field(ioport_field &newfield, ioport_value &disallowedbits, std::string &errorbuf);
+	void insert_field(ioport_field &newfield, ioport_value &disallowedbits, std::ostream &errorbuf);
 
 	// internal state
 	ioport_port *               m_next;         // pointer to next port
@@ -1033,7 +1035,7 @@ class ioport_configurer
 {
 public:
 	// construction/destruction
-	ioport_configurer(device_t &owner, ioport_list &portlist, std::string &errorbuf);
+	ioport_configurer(device_t &owner, ioport_list &portlist, std::ostream &errorbuf);
 
 	// static helpers
 	static const char *string_from_token(const char *string);
@@ -1083,7 +1085,7 @@ class ioport_configurer
 	// internal state
 	device_t &          m_owner;
 	ioport_list &       m_portlist;
-	std::string &       m_errorbuf;
+	std::ostream &      m_errorbuf;
 
 	ioport_port *       m_curport;
 	ioport_field *      m_curfield;
@@ -1123,7 +1125,7 @@ class ioport_configurer
 
 // start of table
 #define INPUT_PORTS_START(_name) \
-ATTR_COLD void INPUT_PORTS_NAME(_name)(device_t &owner, ioport_list &portlist, std::string &errorbuf) \
+ATTR_COLD void INPUT_PORTS_NAME(_name)(device_t &owner, ioport_list &portlist, std::ostream &errorbuf) \
 { \
 	ioport_configurer configurer(owner, portlist, errorbuf);
 // end of table
@@ -1132,7 +1134,7 @@ ATTR_COLD void INPUT_PORTS_NAME(_name)(device_t &owner, ioport_list &portlist, s
 
 // aliasing
 #define INPUT_PORTS_EXTERN(_name) \
-	extern void INPUT_PORTS_NAME(_name)(device_t &owner, ioport_list &portlist, std::string &errorbuf)
+	extern void INPUT_PORTS_NAME(_name)(device_t &owner, ioport_list &portlist, std::ostream &errorbuf)
 
 // including
 #define PORT_INCLUDE(_name) \

src/emu/validity.cpp

@@ -22,6 +22,7 @@
 #include "unicode.h"
 
 #include <cctype>
+#include <sstream>
 #include <type_traits>
 #include <typeinfo>
 
@@ -2495,12 +2496,13 @@ void validity_checker::validate_inputs(device_t &root)
 
 		// allocate the input ports
 		ioport_list portlist;
-		std::string errorbuf;
-		portlist.append(device, errorbuf);
-
-		// report any errors during construction
-		if (!errorbuf.empty())
-			osd_printf_error("I/O port error during construction:\n%s\n", errorbuf);
+		{
+			// report any errors during construction
+			std::ostringstream errorbuf;
+			portlist.append(device, errorbuf);
+			if (errorbuf.tellp())
+				osd_printf_error("I/O port error during construction:\n%s\n", std::move(errorbuf).str());
+		}
 
 		// do a first pass over ports to add their names and find duplicates
 		for (auto &port : portlist)