mu-sexp: improve corner-cases, performance

Some C++20 updates; use the new ascii/ctype functions. Avoid unlimited
recursion. Speed-up conversion to_string

Update unit-tests.

lib/utils/mu-sexp.cc

@@ -21,47 +21,45 @@
 #include "mu-sexp.hh"
 #include "mu-utils.hh"
 
-#include <atomic>
+#include <charconv>
-#include <sstream>
-#include <array>
 
 using namespace Mu;
 
+/* avoid unbounded recursion (i.e., stack overflow) for pathological input */
+constexpr size_t MaxDepth{1024};
+
 template<typename...T> static Mu::Error
 parsing_error(size_t pos, fmt::format_string<T...> frm, T&&... args)
 {
 	const auto&& msg{fmt::format(frm, std::forward<T>(args)...)};
-	if (pos == 0)
+	return Mu::Error(Error::Code::Parsing, "{}: {}", pos, msg);
-		return Mu::Error(Error::Code::Parsing, "{}", msg);
-	else
-		return Mu::Error(Error::Code::Parsing, "{}: {}", pos, msg);
 }
 
 static size_t
 skip_whitespace(const std::string& s, size_t pos)
 {
-	while (pos != s.size()) {
+	while (pos != s.size() && is_ascii_space(s[pos]))
-		if (s[pos] == ' ' || s[pos] == '\t' || s[pos] == '\n')
+		++pos;
-			++pos;
+
-		else
-			break;
-	}
 	return pos;
 }
 
-static Result<Sexp> parse(const std::string& expr, size_t& pos);
+static Result<Sexp> parse(const std::string& expr, size_t& pos, size_t depth);
 
 static Result<Sexp>
-parse_list(const std::string& expr, size_t& pos)
+parse_list(const std::string& expr, size_t& pos, size_t depth)
 {
 	if (expr[pos] != '(') // sanity check.
-		return Err(parsing_error(pos, "expected: '(' but got '{}", expr[pos]));
+		return Err(parsing_error(pos, "expected: '(' but got '{}'", expr[pos]));
+
+	if (depth >= MaxDepth)
+		return Err(parsing_error(pos, "parentheses nested too deeply"));
 
 	Sexp lst{};
 
 	++pos;
 	while (pos < expr.size() && expr[pos] != ')') {
-		if (auto&& item = parse(expr, pos); item)
+		if (auto&& item = parse(expr, pos, depth + 1); item)
 			lst.add(std::move(*item));
 		else
 			return Err(item.error());
@@ -80,27 +78,30 @@ static Result<Sexp>
 parse_string(const std::string& expr, size_t& pos)
 {
 	if (expr[pos] != '"') // sanity check.
-		return Err(parsing_error(pos, "expected: '\"'' but got '{}", expr[pos]));
+		return Err(parsing_error(pos, "expected: '\"' but got '{}'", expr[pos]));
 
 	bool        escape{};
 	std::string str;
 	for (++pos; pos != expr.size(); ++pos) {
 		auto kar = expr[pos];
-		if (escape && (kar == '"' || kar == '\\')) {
+		if (escape) {
-			str += kar;
+			// follow the elisp reader: '\n', '\t' are special,
+			// any other escaped character stands for itself.
+			switch (kar) {
+			case 'n': str += '\n'; break;
+			case 't': str += '\t'; break;
+			default:  str += kar; break;
+			}
 			escape = false;
-			continue;
+		} else if (kar == '\\')
-		}
-
-		if (kar == '"')
-			break;
-		else if (kar == '\\')
 			escape = true;
+		else if (kar == '"')
+			break;
 		else
 			str += kar;
 	}
 
-	if (escape || expr[pos] != '"')
+	if (escape || pos == expr.size())
 		return Err(parsing_error(pos, "unterminated string '{}'", str));
 
 	++pos;
@@ -111,54 +112,57 @@ parse_string(const std::string& expr, size_t& pos)
 static Result<Sexp>
 parse_integer(const std::string& expr, size_t& pos)
 {
-	if (!isdigit(expr[pos]) && expr[pos] != '-') // sanity check.
+	if (!is_ascii_digit(expr[pos]) && expr[pos] != '-') // sanity check.
-		return Err(parsing_error(pos, "expected: <digit> but got '{}", expr[pos]));
+		return Err(parsing_error(pos, "expected: <digit> but got '{}'", expr[pos]));
 
-	std::string num; // negative number?
+	auto end{pos + (expr[pos] == '-' ? 1U : 0U)};
-	if (expr[pos] == '-') {
+	while (end != expr.size() && is_ascii_digit(expr[end]))
-		num = "-";
+		++end;
-		++pos;
-	}
 
-	for (; isdigit(expr[pos]); ++pos)
+	Sexp::Number num{};
-		num += expr[pos];
+	const auto res{std::from_chars(expr.data() + pos, expr.data() + end, num)};
+	if (res.ec != std::errc{} || res.ptr != expr.data() + end)
+		return Err(parsing_error(pos, "invalid number '{}'",
+					 expr.substr(pos, end - pos)));
 
-	return Ok(Sexp{::atoi(num.c_str())});
+	pos = end;
+	return Ok(Sexp{num});
 }
 
 static Result<Sexp>
 parse_symbol(const std::string& expr, size_t& pos)
 {
-	if (!isalpha(expr[pos]) && expr[pos] != ':') // sanity check.
+	if (!is_ascii_alpha(expr[pos]) && expr[pos] != ':') // sanity check.
-		return Err(parsing_error(pos, "expected: <alpha>|: but got '{}", expr[pos]));
+		return Err(parsing_error(pos, "expected: <alpha>|: but got '{}'", expr[pos]));
 
-	std::string symb(1, expr[pos]);
+	const auto start{pos};
-	for (++pos; isalnum(expr[pos]) || expr[pos] == '-'; ++pos)
+	for (++pos; pos != expr.size() &&
-		symb += expr[pos];
+		     (is_ascii_alnum(expr[pos]) || expr[pos] == '-'); ++pos)
+		;
 
-	return Ok(Sexp{Sexp::Symbol{symb}});
+	return Ok(Sexp{Sexp::Symbol{expr.substr(start, pos - start)}});
 }
 
 static Result<Sexp>
-parse(const std::string& expr, size_t& pos)
+parse(const std::string& expr, size_t& pos, size_t depth)
 {
 	pos = skip_whitespace(expr, pos);
 
 	if (pos == expr.size())
-		return Err(parsing_error(pos, "expected: character '{}", expr[pos]));
+		return Err(parsing_error(pos, "unexpected end of input"));
 
 	const auto kar  = expr[pos];
 	const auto sexp = std::invoke([&]() -> Result<Sexp> {
 		if (kar == '(')
-			return parse_list(expr, pos);
+			return parse_list(expr, pos, depth);
 		else if (kar == '"')
 			return parse_string(expr, pos);
-		else if (isdigit(kar) || kar == '-')
+		else if (is_ascii_digit(kar) || kar == '-')
 			return parse_integer(expr, pos);
-		else if (isalpha(kar) || kar == ':')
+		else if (is_ascii_alpha(kar) || kar == ':')
 			return parse_symbol(expr, pos);
 		else
-			return Err(parsing_error(pos, "unexpected character '{}", kar));
+			return Err(parsing_error(pos, "unexpected character '{}'", kar));
 	});
 
 	if (sexp)
@@ -171,7 +175,7 @@ Result<Sexp>
 Sexp::parse(const std::string& expr)
 {
 	size_t pos{};
-	auto res = ::parse(expr, pos);
+	auto res = ::parse(expr, pos, 0/*depth*/);
 	if (!res)
 		return res;
 	else if (pos != expr.size())
@@ -183,31 +187,39 @@ Sexp::parse(const std::string& expr)
 std::string
 Sexp::to_string(Format fopts) const
 {
-	std::stringstream sstrm;
+	std::string str;
-	const auto splitp{any_of(fopts & Format::SplitList)};
+	to_string_into(str, fopts);
-	const auto typeinfop{any_of(fopts & Format::TypeInfo)};
 
+	return str;
+}
+
+void
+Sexp::to_string_into(std::string& out, Format fopts) const
+{
 	if (listp()) {
-		sstrm << '(';
+		out += '(';
 		bool first{true};
 		for(auto&& elm: list()) {
-			sstrm << (first ? "" : " ") << elm.to_string(fopts);
+			if (!first)
+				out += ' ';
+			elm.to_string_into(out, fopts);
 			first = false;
 		}
-		sstrm << ')';
+		out += ')';
-		if (splitp)
+		if (any_of(fopts & Format::SplitList))
-			sstrm << '\n';
+			out += '\n';
 	} else if (stringp())
-		sstrm << quote(string());
+		out += quote(string());
 	else if (numberp())
-		sstrm << number();
+		out += std::to_string(number());
 	else if (symbolp())
-		sstrm << symbol().name;
+		out += symbol().name;
 
-	if (typeinfop)
+	if (any_of(fopts & Format::TypeInfo)) {
-		sstrm << '<' << Sexp::type_name(type())  << '>';
+		out += '<';
-
+		out += Sexp::type_name(type());
-	return sstrm.str();
+		out += '>';
+	}
 }
 
 // LCOV_EXCL_START
@@ -226,8 +238,15 @@ unix_tstamp(const Sexp& emacs_tstamp)
 std::string
 Sexp::to_json_string(Format fopts) const
 {
-	std::stringstream sstrm;
+	std::string str;
+	to_json_string_into(str, fopts);
 
+	return str;
+}
+
+void
+Sexp::to_json_string_into(std::string& out, Format fopts) const
+{
 	const auto sym_name=[&](const std::string& sym) {
 		if (any_of(fopts & Format::NoColon) && sym[0] == ':')
 			return sym.substr(1); // remove colon
@@ -239,60 +258,64 @@ Sexp::to_json_string(Format fopts) const
 	case Type::List: {
 		// property-lists become JSON objects
 		if (plistp()) {
-			sstrm << "{";
+			out += '{';
 			auto it{list().begin()};
 			bool first{true};
 			while (it != list().end()) {
 				const auto key{it->symbol().name};
-				sstrm << (first ? "" : ",")
+				if (!first)
-				      << quote(sym_name(key)) << ":";
+					out += ',';
+				out += quote(sym_name(key));
+				out += ':';
 				++it;
-				const auto emacs_tstamp{*it};
+				const auto& propval{*it};
-				sstrm << emacs_tstamp.to_json_string(fopts);
+				propval.to_json_string_into(out, fopts);
 				++it;
 				first = false;
 				// special-case: tstamp-fields also get a "unix" value,
 				// which are easier to work with than the "emacs" timestamps
-				if (key == ":date" || key == ":changed")
+				if (key == ":date" || key == ":changed") {
-					sstrm << "," << quote(sym_name(key) + "-unix")
+					out += ',';
-					      << ":"
+					out += quote(sym_name(key) + "-unix");
-					      << unix_tstamp(emacs_tstamp);
+					out += ':';
+					out += std::to_string(unix_tstamp(propval));
+				}
 			}
-			sstrm << "}";
+			out += '}';
 			if (any_of(fopts & Format::SplitList))
-			sstrm << '\n';
+				out += '\n';
 		} else { // other lists become arrays.
-			sstrm << '[';
+			out += '[';
 			bool first{true};
 			for (auto&& child : list()) {
-				sstrm << (first ? "" : ", ") << child.to_json_string(fopts);
+				if (!first)
+					out += ", ";
+				child.to_json_string_into(out, fopts);
 				first = false;
 			}
-			sstrm << ']';
+			out += ']';
 			if (any_of(fopts & Format::SplitList))
-				sstrm << '\n';
+				out += '\n';
 		}
 		break;
 	}
 	case Type::String:
-		sstrm << quote(string());
+		out += quote(string());
 		break;
 	case Type::Symbol:
 		if (nilp())
-			sstrm << "false";
+			out += "false";
 		else if (symbol() == "t")
-			sstrm << "true";
+			out += "true";
 		else
-			sstrm << quote(symbol().name);
+			out += quote(symbol().name);
 		break;
 	case Type::Number:
-		sstrm << number();
+		out += std::to_string(number());
 		break;
 	default:
 		break;
 	}
-
-	return sstrm.str();
 }
 
 
@@ -511,6 +534,21 @@ test_parser()
 	check_parse(R"("foo
 bar")",
 		    "\"foo\nbar\"");
+
+	// escapes: '\n'/'\t' are special; \\ and unknown escapes
+	// stand for the escaped character itself
+	check_parse(R"("hello\nworld")", "\"hello\nworld\"");
+	check_parse(R"("tab\there")", "\"tab\there\"");
+	check_parse(R"("back\\slash")", "\"back\\\\slash\"");
+	check_parse(R"("what\qever")", "\"whatqever\"");
+
+	// full int64 range
+	check_parse("4294967296", "4294967296");
+	check_parse("9223372036854775807", "9223372036854775807");
+	check_parse("-9223372036854775808", "-9223372036854775808");
+
+	// \r\n is whitespace, too
+	check_parse("(foo\r\nbar)", "(foo bar)");
 }
 
 static void
@@ -522,6 +560,19 @@ test_parser_fail()
 	g_assert_false(!!Sexp::parse(")"));
 	g_assert_false(!!Sexp::parse("(hello (boo))))"));
 
+	// numbers that don't fit an int64, or aren't numbers at all
+	g_assert_false(!!Sexp::parse("99999999999999999999"));
+	g_assert_false(!!Sexp::parse("-"));
+
+	// trailing backslash
+	g_assert_false(!!Sexp::parse(R"("foo\)"));
+
+	// deeply-nested input gives an error, not a stack overflow
+	std::string deep(100000, '(');
+	deep += "a";
+	deep.append(100000, ')');
+	g_assert_false(!!Sexp::parse(deep));
+
 	g_assert_true(Sexp::type_name(static_cast<Sexp::Type>(-1)) == "<error>");
 }
 

lib/utils/mu-sexp.hh

@@ -25,9 +25,9 @@
 #include <vector>
 #include <string>
 #include <string_view>
-#include <iostream>
 #include <variant>
 #include <ostream>
+#include <type_traits>
 
 #include <utils/mu-result.hh>
 #include <utils/mu-option.hh>
@@ -56,13 +56,20 @@ struct Sexp {
 		operator const std::string&() const {return name; }
 		std::string name;
 
-		bool operator==(const Symbol& rhs) const {
+		bool operator==(const Symbol& rhs) const = default;
-			return this == &rhs ? true : rhs.name == name;
-		}
-		bool operator!=(const Symbol& rhs) const { return *this == rhs ? false : true; }
 	};
 	enum struct Type { List, String, Number, Symbol };
 	using ValueType = std::variant<List, String, Number, Symbol>;
+	// the Type enum and the ValueType variant must be kept in sync,
+	// since type() merely casts the variant-index.
+	static_assert(std::is_same_v<std::variant_alternative_t<
+			      static_cast<size_t>(Type::List), ValueType>, List>);
+	static_assert(std::is_same_v<std::variant_alternative_t<
+			      static_cast<size_t>(Type::String), ValueType>, String>);
+	static_assert(std::is_same_v<std::variant_alternative_t<
+			      static_cast<size_t>(Type::Number), ValueType>, Number>);
+	static_assert(std::is_same_v<std::variant_alternative_t<
+			      static_cast<size_t>(Type::Symbol), ValueType>, Symbol>);
 
 	/**
 	 * Is some Sexp of the given type?
@@ -91,8 +98,8 @@ struct Sexp {
 	 */
 	Sexp():value{List{}} {} // default: an empty list.
 	// Copy & move ctors
-	Sexp(const Sexp& other):value{other.value}{}
+	Sexp(const Sexp& other) = default;
-	Sexp(Sexp&& other):value{std::move(other.value)}{}
+	Sexp(Sexp&& other) = default;
 	// From various types
 	Sexp(const List& lst): value{lst} {}
 	Sexp(List&& lst): value{std::move(lst)} {}
@@ -118,31 +125,9 @@ struct Sexp {
 #pragma GCC diagnostic pop
 	}
 
-	/**
+	// Copy & move assignment
-	 * Copy-assignment
+	Sexp& operator=(const Sexp& rhs) = default;
-	 *
+	Sexp& operator=(Sexp&& rhs) = default;
-	 * @param rhs another sexp
-	 *
-	 * @return the sexp
-	 */
-	Sexp& operator=(const Sexp& rhs) {
-		if (this != &rhs)
-			value = rhs.value;
-		return *this;
-	}
-
-	/**
-	 * Move-assignment
-	 *
-	 * @param rhs another sexp
-	 *
-	 * @return the sexp
-	 */
-	Sexp& operator=(Sexp&& rhs) {
-		if (this != &rhs)
-			value = std::move(rhs.value);
-		return *this;
-	}
 
 	/**
 	 * Get the type of value
@@ -207,7 +192,7 @@ struct Sexp {
 	Sexp& add()              { return *this; }
 
 	template <typename V1, typename V2, typename... Args>
-	Sexp& add(V1&& v1, V2&& v2, Args... args) {
+	Sexp& add(V1&& v1, V2&& v2, Args&&... args) {
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
 		return add(std::forward<V1>(v1))
@@ -241,7 +226,7 @@ struct Sexp {
 	bool plistp() const { return listp() && plistp(cbegin(), cend()); }
 	Sexp& put_props() { return *this; } // Final case for template pack.
 	template <class PropType, class SexpType, typename... Args>
-	Sexp& put_props(PropType&& prop, SexpType&& sexp, Args... args) {
+	Sexp& put_props(PropType&& prop, SexpType&& sexp, Args&&... args) {
 		auto&& propname{std::string(prop)};
 		return del_prop(propname)
 			.add(Symbol(std::move(propname)),
@@ -256,7 +241,7 @@ struct Sexp {
 	 *
 	 * @return the property if found, or nothing
 	 */
-	const Option<const Sexp&> get_prop(const std::string& p) const {
+	Option<const Sexp&> get_prop(const std::string& p) const {
 		if (auto&& it = find_prop(p, cbegin(), cend()); it != cend())
 			return *(std::next(it));
 		else
@@ -295,6 +280,9 @@ protected:
 private:
 	iterator find_prop(const std::string& s,iterator b,
 				 iterator e);
+	void to_string_into(std::string& out, Format fopts) const;
+	void to_json_string_into(std::string& out, Format fopts) const;
+
 	ValueType value;
 
 
@@ -308,7 +296,7 @@ MU_ENABLE_BITOPS(Sexp::Format);
 inline Sexp::Symbol
 operator""_sym(const char* str, std::size_t n)
 {
-	return Sexp::Symbol{str};
+	return Sexp::Symbol{std::string{str, n}};
 }
 
 inline std::ostream&