Code review comments

Author: VincentLascaux

inc/goldfish/common.h

@@ -38,4 +38,9 @@ namespace goldfish
 	// We implement our own that forwards to VC++ implementation or is identity depending on the compiler
 	template <class T> auto make_unchecked_array_iterator(T&& t) { return stdext::make_unchecked_array_iterator(std::forward<T>(t)); }
 	template <class T> auto get_array_iterator_from_unchecked(T&& t) { return t.base(); }
+
+	template <size_t...> struct largest {};
+	template <size_t x> struct largest<x> { enum { value = x }; };
+	template <size_t x, size_t y> struct largest<x, y> { enum { value = x > y ? x : y }; };
+	template <size_t x, size_t y, size_t... z> struct largest<x, y, z...> { enum { value = largest<x, largest<y, z...>::value>::value }; };
 }

inc/goldfish/iostream_adaptor.h

@@ -18,7 +18,7 @@ namespace goldfish { namespace stream
 			if (m_stream.bad() || (m_stream.fail() && !m_stream.eof()))
 				throw io_exception{};
 
-			return m_stream.gcount();
+			return static_cast<size_t>(m_stream.gcount());
 		}
 	private:
 		std::istream& m_stream;

inc/goldfish/sax_writer.h

@@ -178,7 +178,7 @@ namespace goldfish { namespace sax
 			));
 		}
 
-		template <class T> decltype(serialize_to_goldfish(std::declval<document_writer<inner>&>(), std::declval<T&>())) write(T&& t)
+		template <class T> decltype(serialize_to_goldfish(std::declval<document_writer<inner>&>(), std::declval<T&&>())) write(T&& t)
 		{
 			return serialize_to_goldfish(*this, std::forward<T>(t));
 		}

inc/goldfish/schema.h

@@ -9,7 +9,7 @@ namespace goldfish
 {
 	namespace details
 	{
-		template <size_t N> const_buffer_ref make_key(const char(&text)[N])
+		template <size_t N> constexpr const_buffer_ref make_key(const char(&text)[N])
 		{
 			static_assert(N > 0, "expect null terminated strings");
 			assert(text[N - 1] == 0);
@@ -22,11 +22,11 @@ namespace goldfish
 				: m_keys{ std::forward<Args>(args)... }
 			{}
 
-			template <size_t N> optional<size_t> search_text(const char(&text)[N]) const
+			optional<size_t> search_key(const_buffer_ref key, size_t start_index) const
 			{
-				return search_impl({ reinterpret_cast<const byte*>(text), N - 1 }, std::integral_constant<size_t, 0>{});
+				return search_impl(key, start_index);
 			}
-			template <class Document> std::enable_if_t<tags::has_tag<Document, tags::document>::value, optional<size_t>> search(Document& d) const
+			template <class Document> std::enable_if_t<tags::has_tag<Document, tags::document>::value, optional<size_t>> search(Document& d, size_t start_index) const
 			{
 				return d.visit(first_match(
 					[&](auto& text, tags::string) -> optional<size_t>
@@ -36,7 +36,7 @@ namespace goldfish
 						if (stream::seek(text, std::numeric_limits<uint64_t>::max()) != 0)
 							return nullopt;
 
-						return search_impl({ buffer, length }, std::integral_constant<size_t, 0>{});
+						return search_impl({ buffer, length }, start_index);
 					},
 					[&](auto&, auto) -> optional<size_t>
 					{
@@ -45,9 +45,9 @@ namespace goldfish
 					}));
 			}
 		private:
-			template <size_t cur_index> optional<size_t> search_impl(const_buffer_ref text, std::integral_constant<size_t, cur_index>) const
+			optional<size_t> search_impl(const_buffer_ref text, size_t start_index) const
 			{
-				auto it = std::find_if(m_keys.begin(), m_keys.end(), [&](auto&& key)
+				auto it = std::find_if(m_keys.begin() + start_index, m_keys.end(), [&](auto&& key)
 				{
 					return key.size() == text.size() && std::equal(key.begin(), key.end(), make_unchecked_array_iterator(text.begin()));
 				});
@@ -60,9 +60,12 @@ namespace goldfish
 			std::array<const_buffer_ref, N> m_keys;
 		};
 	}
-	template <class... T> auto make_schema(T&&... keys)
+	template <class... T> constexpr auto make_schema(T&&... keys)
 	{
-		return details::schema<sizeof...(T), 1024>(details::make_key(std::forward<T>(keys))...);
+		return details::schema<
+			sizeof...(T), 
+			largest<sizeof(T)...>::value - 1
+		>(details::make_key(std::forward<T>(keys))...);
 	}
 
 	template <class Map, class Schema> class map_with_schema
@@ -72,8 +75,14 @@ namespace goldfish
 			: m_map(std::move(map))
 			, m_schema(schema)
 		{}
-		optional<decltype(std::declval<Map>().read_value())> read_value_by_index(size_t index)
+		optional<decltype(std::declval<Map>().read_value())> read_by_schema_index(size_t index)
 		{
+			#ifndef NDEBUG
+			if (m_last_queried_index)
+				assert(*m_last_queried_index < index);
+			m_last_queried_index = index;
+			#endif
+
 			if (m_index > index)
 				return nullopt;
 
@@ -89,7 +98,7 @@ namespace goldfish
 
 			while (auto key = m_map.read_key())
 			{
-				if (auto new_index = m_schema.search(*key))
+				if (auto new_index = m_schema.search(*key, m_index /*start_index*/))
 				{
 					m_index = *new_index;
 				}
@@ -116,17 +125,17 @@ namespace goldfish
 				}
 				else
 				{
-					// We found a key that is still before us, skip the value and keep searching
+					// We found a key that is still before the one we are looking for, skip the value and keep searching
 					seek_to_end(m_map.read_value());
 				}
 			}
 
 			return nullopt;
 		}
-		template <size_t N> auto read_value(const char(&text)[N])
+		template <class Key> auto read(Key&& key)
 		{
-			if (auto index = m_schema.search_text(text))
-				return read_value_by_index(*index);
+			if (auto index = m_schema.search_key(details::make_key(std::forward<Key>(key)), 0 /*start_index*/))
+				return read_by_schema_index(*index);
 			else
 				std::terminate();			
 		}
@@ -145,6 +154,10 @@ namespace goldfish
 		Schema m_schema;
 		size_t m_index = 0;
 		bool m_on_value = false;
+
+		#ifndef NDEBUG
+		optional<size_t> m_last_queried_index;
+		#endif
 	};
 	template <class Map, class Schema> map_with_schema<std::decay_t<Map>, Schema> apply_schema(Map&& map, const Schema& s)
 	{

inc/goldfish/variant.h

@@ -23,11 +23,6 @@ namespace goldfish
 
 		template <class T> struct negate { enum { value = !T::value }; };
 
-		template <size_t...> struct largest {};
-		template <size_t x> struct largest<x> { enum { value = x }; };
-		template <size_t x, size_t y> struct largest<x, y> { enum { value = x > y ? x : y }; };
-		template <size_t x, size_t y, size_t... z> struct largest<x, y, z...> { enum { value = largest<x, largest<y, z...>::value>::value }; };
-
 		template <class T, class dummy = uint8_t> struct with_padding
 		{
 			T data;

tests/schema.cpp

@@ -9,7 +9,7 @@ namespace goldfish
 	{
 		auto map = apply_schema(json::read(stream::read_string_non_owning("{}")).as_map(), make_schema("10", "20", "30"));
 
-		test(map.read_value_by_index(0) == nullopt);
+		test(map.read_by_schema_index(0) == nullopt);
 
 		seek_to_end(map);
 	}
@@ -21,24 +21,24 @@ namespace goldfish
 			make_schema("10", "20", "30", "40", "50", "60", "70", "80", "90"));
 
 		// Reading the very first key
-		test(dom::load_in_memory(*map.read_value_by_index(0)) == 1ull);
+		test(dom::load_in_memory(*map.read_by_schema_index(0)) == 1ull);
 
 		// Reading index 1 will force to skip the entry 15 and go to entry 40
-		test(map.read_value_by_index(1) == nullopt);
+		test(map.read_by_schema_index(1) == nullopt);
 
 		// Reading index 2 will fail because we are already at index 3 of the schema
-		test(map.read_value_by_index(2) == nullopt);
+		test(map.read_by_schema_index(2) == nullopt);
 
 		// We are currently at index 3 but are asking for index 5, that should skip the pair 40:3 and 50:4 and find 60:5
-		test(dom::load_in_memory(*map.read_value_by_index(5)) == 5ull);
+		test(dom::load_in_memory(*map.read_by_schema_index(5)) == 5ull);
 
 		// We ask for index 6, which brings to index 7 (and returns null)
 		// Asking for index 7 should return the value on an already read key
-		test(map.read_value_by_index(6) == nullopt);
-		test(dom::load_in_memory(*map.read_value_by_index(7)) == 6ull);
+		test(map.read_by_schema_index(6) == nullopt);
+		test(dom::load_in_memory(*map.read_by_schema_index(7)) == 6ull);
 
 		// finally, ask for index 8, but we reach the end of the map before we find it
-		test(map.read_value_by_index(8) == nullopt);
+		test(map.read_by_schema_index(8) == nullopt);
 
 		seek_to_end(map);
 	}
@@ -49,14 +49,14 @@ namespace goldfish
 			json::read(stream::read_string_non_owning("{\"20\":1}")).as_map(),
 			make_schema("10", "20"));
 
-		test(map.read_value_by_index(0) == nullopt);
+		test(map.read_by_schema_index(0) == nullopt);
 		seek_to_end(map);
 	}
 
 	TEST_CASE(test_filtered_map_by_value)
 	{
 		auto map = apply_schema(json::read(stream::read_string_non_owning("{\"B\":1}")).as_map(), make_schema("A", "B"));
-		test(dom::load_in_memory(*map.read_value("B")) == 1ull);
+		test(dom::load_in_memory(*map.read("B")) == 1ull);
 		seek_to_end(map);
 	}
 }

tests/tutorial.cpp

@@ -54,9 +54,9 @@ TEST_CASE(parse_complex)
 	while (auto entry_document = document.read())
 	{
 		auto entry = entry_document->as_map("name", "friends");
-		output << entry.read_value("name").value().as_string() << " has the following friends: ";
+		output << entry.read("name").value().as_string() << " has the following friends: ";
 
-		auto friends = entry.read_value("friends").value().as_array();
+		auto friends = entry.read("friends").value().as_array();
 		while (auto friend_name = friends.read())
 			output << friend_name->as_string() << " ";