Merge pull request #194 from wravery/warning-flags

wravery · web-flow · commit 7d5633803f7d · 2022-01-06T17:55:06.000-08:00
Enable strict compiler warnings and fix resulting warnings/errors
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -6,6 +6,9 @@ cmake_minimum_required(VERSION 3.15)
 # Enable CMAKE_MSVC_RUNTIME_LIBRARY on Windows: https://cmake.org/cmake/help/latest/policy/CMP0091.html
 cmake_policy(SET CMP0091 NEW)
 
+# Do not set default MSVC warning flags: https://cmake.org/cmake/help/latest/policy/CMP0092.html
+cmake_policy(SET CMP0092 NEW)
+
 # Default to the last updated version from version.txt.
 file(TO_CMAKE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake/version.txt" VERSION_FILE)
 file(READ "${VERSION_FILE}" LATEST_VERSION)
@@ -52,6 +55,13 @@ if(VCPKG_TARGET_TRIPLET)
   endif()
 endif()
 
+# Enable more warnings, and treat warnings as errors.
+if(MSVC)
+  add_compile_options(/W4 /WX /permissive-)
+else()
+  add_compile_options(-Wall -Wextra -pedantic -Werror)
+endif()
+
 function(add_bigobj_flag target)
   if(MSVC)
     # MSVC requires the /bigobj flag if the number of sections gets too big.
diff --git a/include/SchemaLoader.h b/include/SchemaLoader.h
@@ -48,7 +48,7 @@ struct ScalarType
 {
 	std::string_view type;
 	std::string_view description;
-	std::string_view specifiedByURL;
+	std::string_view specifiedByURL {};
 };
 
 using ScalarTypeList = std::vector<ScalarType>;
@@ -105,7 +105,7 @@ struct InputType
 	std::string_view cppType;
 	InputFieldList fields;
 	std::string_view description;
-	std::unordered_set<std::string_view> dependencies;
+	std::unordered_set<std::string_view> dependencies {};
 };
 
 using InputTypeList = std::vector<InputType>;
@@ -298,8 +298,8 @@ class SchemaLoader
 	void validateImplementedInterfaces() const;
 	const InterfaceType& findInterfaceType(
 		std::string_view typeName, std::string_view interfaceName) const;
-	void validateInterfaceFields(std::string_view typeName,
-		std::string_view interfaceName, const OutputFieldList& typeFields) const;
+	void validateInterfaceFields(std::string_view typeName, std::string_view interfaceName,
+		const OutputFieldList& typeFields) const;
 	void validateTransitiveInterfaces(
 		std::string_view typeName, const std::vector<std::string_view>& interfaces) const;
 
diff --git a/include/graphqlservice/GraphQLService.h b/include/graphqlservice/GraphQLService.h
@@ -76,8 +76,8 @@ GRAPHQLSERVICE_EXPORT error_path buildErrorPath(const std::optional<field_path>&
 struct schema_error
 {
 	std::string message;
-	schema_location location;
-	error_path path;
+	schema_location location {};
+	error_path path {};
 };
 
 GRAPHQLSERVICE_EXPORT response::Value buildErrorValues(std::list<schema_error>&& structuredErrors);
@@ -579,7 +579,7 @@ struct ResolverParams : SelectionSetParams
 struct ResolverResult
 {
 	response::Value data;
-	std::list<schema_error> errors;
+	std::list<schema_error> errors {};
 };
 
 using AwaitableResolver = internal::Awaitable<ResolverResult>;
@@ -1195,10 +1195,10 @@ struct RequestResolveParams
 	response::Value variables { response::Type::Map };
 
 	// Optional async execution awaitable.
-	await_async launch;
+	await_async launch {};
 
 	// Optional sub-class of RequestState which will be passed to each resolver and field accessor.
-	std::shared_ptr<RequestState> state;
+	std::shared_ptr<RequestState> state {};
 };
 
 struct RequestSubscribeParams
@@ -1212,10 +1212,10 @@ struct RequestSubscribeParams
 	response::Value variables { response::Type::Map };
 
 	// Optional async execution awaitable.
-	await_async launch;
+	await_async launch {};
 
 	// Optional sub-class of RequestState which will be passed to each resolver and field accessor.
-	std::shared_ptr<RequestState> state;
+	std::shared_ptr<RequestState> state {};
 };
 
 struct RequestUnsubscribeParams
@@ -1224,7 +1224,7 @@ struct RequestUnsubscribeParams
 	SubscriptionKey key;
 
 	// Optional async execution awaitable.
-	await_async launch;
+	await_async launch {};
 };
 
 using SubscriptionArguments = std::map<std::string_view, response::Value>;
@@ -1240,7 +1240,7 @@ struct SubscriptionFilter
 
 	// Optional field directives filter, which can either be a set of required directives and
 	// arguments, or a callback which returns true if the directives match custom criteria.
-	std::optional<std::variant<Directives, SubscriptionDirectiveFilterCallback>> directives;
+	std::optional<std::variant<Directives, SubscriptionDirectiveFilterCallback>> directives {};
 };
 
 // Deliver to a specific subscription key, or apply custom criteria for the field name, arguments,
@@ -1254,13 +1254,13 @@ struct RequestDeliverParams
 
 	// Optional filter to control which subscriptions will receive the event. If not specified,
 	// every subscription on this field will receive the event and evaluate their queries.
-	RequestDeliverFilter filter;
+	RequestDeliverFilter filter {};
 
 	// Optional async execution awaitable.
-	await_async launch;
+	await_async launch {};
 
 	// Optional override for the default Subscription operation object.
-	std::shared_ptr<Object> subscriptionObject;
+	std::shared_ptr<Object> subscriptionObject {};
 };
 
 using TypeMap = internal::string_view_map<std::shared_ptr<Object>>;
diff --git a/samples/today/CMakeLists.txt b/samples/today/CMakeLists.txt
@@ -15,6 +15,12 @@ add_library(todaygraphql_nointrospection STATIC TodayMock.cpp)
 target_link_libraries(todaygraphql_nointrospection PUBLIC today_nointrospection_schema)
 target_include_directories(todaygraphql_nointrospection PUBLIC ${CMAKE_CURRENT_SOURCE_DIR})
 
+if(MSVC)
+  # warning C4702: unreachable code
+  target_compile_options(todaygraphql PUBLIC /wd4702)
+  target_compile_options(todaygraphql_nointrospection PUBLIC /wd4702)
+endif()
+
 # sample
 add_executable(sample sample.cpp)
 target_link_libraries(sample PRIVATE
diff --git a/samples/today/TodayMock.h b/samples/today/TodayMock.h
@@ -262,7 +262,6 @@ class Task
 	response::IdType _id;
 	std::shared_ptr<const response::Value> _title;
 	bool _isComplete;
-	TaskState _state = TaskState::New;
 };
 
 class TaskEdge
diff --git a/samples/today/sample.cpp b/samples/today/sample.cpp
@@ -61,21 +61,21 @@ int main(int argc, char** argv)
 
 	try
 	{
-		peg::ast query;
+		peg::ast ast;
 
 		if (argc > 1)
 		{
-			query = peg::parseFile(argv[1]);
+			ast = peg::parseFile(argv[1]);
 		}
 		else
 		{
 			std::istream_iterator<char> start { std::cin >> std::noskipws }, end {};
 			std::string input { start, end };
 
-			query = peg::parseString(std::move(input));
+			ast = peg::parseString(std::move(input));
 		}
 
-		if (!query.root)
+		if (!ast.root)
 		{
 			std::cerr << "Unknown error!" << std::endl;
 			std::cerr << std::endl;
@@ -85,7 +85,7 @@ int main(int argc, char** argv)
 		std::cout << "Executing query..." << std::endl;
 
 		std::cout << response::toJSON(
-			service->resolve({ query, ((argc > 2) ? argv[2] : "") }).get())
+			service->resolve({ ast, ((argc > 2) ? argv[2] : "") }).get())
 				  << std::endl;
 	}
 	catch (const std::runtime_error& ex)
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
@@ -42,7 +42,7 @@ if(GRAPHQL_UPDATE_VERSION)
 endif()
 
 if(WIN32)
-  # Always build the version.rc files with these flags, even if we aren't regnerating them.
+  # Always build the version.rc files with these flags, even if we aren't regenerating them.
   set(CMAKE_RC_FLAGS "/nologo")
   set(CMAKE_RC_FLAGS_DEBUG "/dDEBUG")
 
diff --git a/src/ClientGenerator.cpp b/src/ClientGenerator.cpp
@@ -206,8 +206,7 @@ static_assert(graphql::internal::MinorVersion == )cpp"
 	{
 		pendingSeparator.reset();
 
-		headerFile << R"cpp(enum class )cpp" << _schemaLoader.getCppType(enumType->name())
-				   << R"cpp(
+		headerFile << R"cpp(enum class )cpp" << _schemaLoader.getCppType(enumType->name()) << R"cpp(
 {
 )cpp";
 		for (const auto& enumValue : enumType->enumValues())
@@ -459,15 +458,13 @@ using namespace )cpp"
 
 		const auto& enumValues = enumType->enumValues();
 		const auto cppType = _schemaLoader.getCppType(enumType->name());
-		bool firstValue = true;
 
 		sourceFile << R"cpp(static const std::array<std::string_view, )cpp" << enumValues.size()
 				   << R"cpp(> s_names)cpp" << cppType << R"cpp( = {
 )cpp";
 
 		for (const auto& enumValue : enumValues)
 		{
-			firstValue = false;
 			sourceFile << R"cpp(	")cpp" << SchemaLoader::getSafeCppName(enumValue->name())
 					   << R"cpp("sv,
 )cpp";
@@ -485,7 +482,6 @@ using namespace )cpp"
 		{
 			pendingSeparator.reset();
 
-			const auto& enumValues = enumType->enumValues();
 			const auto cppType = _schemaLoader.getCppType(enumType->name());
 
 			if (!variables.empty())
diff --git a/src/GeneratorUtil.cpp b/src/GeneratorUtil.cpp
@@ -21,7 +21,7 @@ IncludeGuardScope::IncludeGuardScope(
 				return '_';
 			}
 
-			return std::toupper(ch);
+			return static_cast<char>(std::toupper(ch));
 		});
 
 	_outputFile << R"cpp(// Copyright (c) Microsoft Corporation. All rights reserved.
diff --git a/src/GraphQLResponse.cpp b/src/GraphQLResponse.cpp
@@ -496,7 +496,7 @@ Value::Value(const Value& other)
 			std::transform(members.cbegin(),
 				members.cend(),
 				std::back_inserter(copy.members),
-				[&copy](const auto& entry) noexcept {
+				[](const auto& entry) noexcept {
 					return entry.second;
 				});
 
diff --git a/src/GraphQLService.cpp b/src/GraphQLService.cpp
@@ -1631,7 +1631,7 @@ std::pair<std::string_view, const peg::ast_node*> Request::findOperationDefiniti
 	std::pair<std::string_view, const peg::ast_node*> result = { {}, nullptr };
 
 	peg::on_first_child_if<peg::operation_definition>(*query.root,
-		[this, &operationName, &result](const peg::ast_node& operationDefinition) noexcept -> bool {
+		[&operationName, &result](const peg::ast_node& operationDefinition) noexcept -> bool {
 			std::string_view operationType = strQuery;
 
 			peg::on_first_child<peg::operation_type>(operationDefinition,
@@ -1706,7 +1706,7 @@ response::AwaitableValue Request::resolve(RequestResolveParams params) const
 		const auto resolverContext =
 			isMutation ? ResolverContext::Mutation : ResolverContext::Query;
 		// https://spec.graphql.org/October2021/#sec-Normal-and-Serial-Execution
-		const auto operationLaunch = isMutation ? await_async {} : params.launch;
+		auto operationLaunch = isMutation ? await_async {} : params.launch;
 
 		OperationDefinitionVisitor operationVisitor(resolverContext,
 			std::move(operationLaunch),
diff --git a/src/RequestLoader.cpp b/src/RequestLoader.cpp
@@ -473,7 +473,7 @@ void RequestLoader::addTypesToSchema()
 			std::transform(objectType.interfaces.cbegin(),
 				objectType.interfaces.cend(),
 				interfaces.begin(),
-				[this, &interfaceTypes](std::string_view interfaceName) noexcept {
+				[&interfaceTypes](std::string_view interfaceName) noexcept {
 					return interfaceTypes[interfaceName];
 				});
 
@@ -845,7 +845,7 @@ void RequestLoader::reorderInputTypeDependencies() noexcept
 	{
 		RequestSchemaType type;
 		std::string_view name;
-		std::unordered_set<std::string_view> dependencies;
+		std::unordered_set<std::string_view> dependencies {};
 	};
 
 	std::vector<InputTypeDependencies> inputTypes(_referencedInputTypes.size());
diff --git a/src/SchemaLoader.cpp b/src/SchemaLoader.cpp
@@ -891,8 +891,6 @@ void SchemaLoader::visitEnumTypeExtension(const peg::ast_node& enumTypeExtension
 
 						if (directiveName == "deprecated"sv)
 						{
-							std::string_view reason;
-
 							peg::on_first_child<peg::arguments>(directive,
 								[&value](const peg::ast_node& arguments) {
 									peg::on_first_child<peg::argument>(arguments,
@@ -1097,7 +1095,7 @@ void SchemaLoader::visitDirectiveDefinition(const peg::ast_node& directiveDefini
 		});
 
 	peg::on_first_child<peg::repeatable_keyword>(directiveDefinition,
-		[&directive](const peg::ast_node& child) {
+		[&directive](const peg::ast_node& /* child */) {
 			directive.isRepeatable = true;
 		});
 
diff --git a/src/Validation.cpp b/src/Validation.cpp
@@ -244,12 +244,12 @@ void ValidateArgumentValueVisitor::visitObjectValue(const peg::ast_node& objectV
 		if (value.values.find(name) != value.values.end())
 		{
 			// https://spec.graphql.org/October2021/#sec-Input-Object-Field-Uniqueness
-			auto position = field->begin();
+			auto fieldPosition = field->begin();
 			std::ostringstream message;
 
 			message << "Conflicting input field name: " << name;
 
-			_errors.push_back({ message.str(), { position.line, position.column } });
+			_errors.push_back({ message.str(), { fieldPosition.line, fieldPosition.column } });
 			continue;
 		}
 
@@ -567,7 +567,6 @@ void ValidateExecutableVisitor::visit(const peg::ast_node& root)
 	if (!_fragmentDefinitions.empty())
 	{
 		// https://spec.graphql.org/October2021/#sec-Fragments-Must-Be-Used
-		const size_t originalSize = _errors.size();
 		auto unreferencedFragments = std::move(_fragmentDefinitions);
 
 		for (const auto& name : _referencedFragments)
@@ -920,7 +919,7 @@ bool ValidateExecutableVisitor::matchesScopedType(std::string_view name) const
 	{
 		const auto itrMatch = std::find_if(itrScoped->second.begin(),
 			itrScoped->second.end(),
-			[this, itrNamed](std::string_view matchingType) noexcept {
+			[itrNamed](std::string_view matchingType) noexcept {
 				return itrNamed->second.find(matchingType) != itrNamed->second.end();
 			});
 
diff --git a/test/ArgumentTests.cpp b/test/ArgumentTests.cpp
@@ -27,7 +27,7 @@ TEST(ArgumentsCase, ListArgumentStrings)
 		FAIL() << response::toJSON(ex.getErrors());
 	}
 
-	ASSERT_EQ(3, actual.size()) << "should get 3 entries";
+	ASSERT_EQ(size_t { 3 }, actual.size()) << "should get 3 entries";
 	EXPECT_EQ("string1", actual[0]) << "entry should match";
 	EXPECT_EQ("string2", actual[1]) << "entry should match";
 	EXPECT_EQ("string3", actual[2]) << "entry should match";
@@ -80,7 +80,7 @@ TEST(ArgumentsCase, ListArgumentStringsNullable)
 		FAIL() << response::toJSON(ex.getErrors());
 	}
 
-	ASSERT_EQ(4, actual.size()) << "should get 4 entries";
+	ASSERT_EQ(size_t { 4 }, actual.size()) << "should get 4 entries";
 	ASSERT_TRUE(actual[0].has_value()) << "should not be null";
 	EXPECT_EQ("string1", *actual[0]) << "entry should match";
 	ASSERT_TRUE(actual[1].has_value()) << "should not be null";
@@ -108,11 +108,11 @@ TEST(ArgumentsCase, ListArgumentListArgumentStrings)
 		FAIL() << response::toJSON(ex.getErrors());
 	}
 
-	ASSERT_EQ(2, actual.size()) << "should get 2 entries";
-	ASSERT_EQ(2, actual[0].size()) << "should get 2 entries";
+	ASSERT_EQ(size_t { 2 }, actual.size()) << "should get 2 entries";
+	ASSERT_EQ(size_t { 2 }, actual[0].size()) << "should get 2 entries";
 	EXPECT_EQ("list1string1", actual[0][0]) << "entry should match";
 	EXPECT_EQ("list1string2", actual[0][1]) << "entry should match";
-	ASSERT_EQ(2, actual[1].size()) << "should get 2 entries";
+	ASSERT_EQ(size_t { 2 }, actual[1].size()) << "should get 2 entries";
 	EXPECT_EQ("list2string1", actual[1][0]) << "entry should match";
 	EXPECT_EQ("list2string2", actual[1][1]) << "entry should match";
 }
@@ -136,9 +136,9 @@ TEST(ArgumentsCase, ListArgumentNullableListArgumentStrings)
 		FAIL() << response::toJSON(ex.getErrors());
 	}
 
-	ASSERT_EQ(2, actual.size()) << "should get 2 entries";
+	ASSERT_EQ(size_t { 2 }, actual.size()) << "should get 2 entries";
 	EXPECT_FALSE(actual[0].has_value()) << "should be null";
-	ASSERT_EQ(2, actual[1]->size()) << "should get 2 entries";
+	ASSERT_EQ(size_t { 2 }, actual[1]->size()) << "should get 2 entries";
 	EXPECT_EQ("list2string1", (*actual[1])[0]) << "entry should match";
 	EXPECT_EQ("list2string2", (*actual[1])[1]) << "entry should match";
 }
@@ -227,7 +227,7 @@ TEST(ArgumentsCase, ScalarArgumentMap)
 
 	ASSERT_EQ(response::Type::Map, actual.type()) << "should parse the object";
 	values = actual.release<response::MapType>();
-	ASSERT_EQ(1, values.size()) << "should have a single key/value";
+	ASSERT_EQ(size_t { 1 }, values.size()) << "should have a single key/value";
 	ASSERT_EQ("foo", values.front().first) << "should match the key";
 	ASSERT_EQ("bar", values.front().second.get<std::string>()) << "should match the value";
 }
@@ -250,7 +250,7 @@ TEST(ArgumentsCase, ScalarArgumentList)
 
 	ASSERT_EQ(response::Type::List, actual.type()) << "should parse the array";
 	values = actual.release<response::ListType>();
-	ASSERT_EQ(2, values.size()) << "should have 2 values";
+	ASSERT_EQ(size_t { 2 }, values.size()) << "should have 2 values";
 	ASSERT_EQ("foo", values.front().get<std::string>()) << "should match the value";
 	ASSERT_EQ("bar", values.back().get<std::string>()) << "should match the value";
 }
diff --git a/test/ClientTests.cpp b/test/ClientTests.cpp
diff --git a/test/NoIntrospectionTests.cpp b/test/NoIntrospectionTests.cpp
diff --git a/test/PegtlCombinedTests.cpp b/test/PegtlCombinedTests.cpp
diff --git a/test/PegtlExecutableTests.cpp b/test/PegtlExecutableTests.cpp
diff --git a/test/PegtlSchemaTests.cpp b/test/PegtlSchemaTests.cpp
diff --git a/test/TodayTests.cpp b/test/TodayTests.cpp
diff --git a/test/ValidationTests.cpp b/test/ValidationTests.cpp

Original file line number	Diff line number	Diff line change
`@@ -61,21 +61,21 @@ int main(int argc, char** argv)`
`61`	`61`
`62`	`62`	`try`
`63`	`63`	`{`
`64`		`- peg::ast query;`
	`64`	`+ peg::ast ast;`
`65`	`65`
`66`	`66`	`if (argc > 1)`
`67`	`67`	`{`
`68`		`- query = peg::parseFile(argv[1]);`
	`68`	`+ ast = peg::parseFile(argv[1]);`
`69`	`69`	`}`
`70`	`70`	`else`
`71`	`71`	`{`
`72`	`72`	`std::istream_iterator<char> start { std::cin >> std::noskipws }, end {};`
`73`	`73`	`std::string input { start, end };`
`74`	`74`
`75`		`- query = peg::parseString(std::move(input));`
	`75`	`+ ast = peg::parseString(std::move(input));`
`76`	`76`	`}`
`77`	`77`
`78`		`- if (!query.root)`
	`78`	`+ if (!ast.root)`
`79`	`79`	`{`
`80`	`80`	`std::cerr << "Unknown error!" << std::endl;`
`81`	`81`	`std::cerr << std::endl;`
`@@ -85,7 +85,7 @@ int main(int argc, char** argv)`
`85`	`85`	`std::cout << "Executing query..." << std::endl;`
`86`	`86`
`87`	`87`	`std::cout << response::toJSON(`
`88`		`- service->resolve({ query, ((argc > 2) ? argv[2] : "") }).get())`
	`88`	`+ service->resolve({ ast, ((argc > 2) ? argv[2] : "") }).get())`
`89`	`89`	`<< std::endl;`
`90`	`90`	`}`
`91`	`91`	`catch (const std::runtime_error& ex)`
Original file line number	Diff line number	Diff line change
`@@ -21,7 +21,7 @@ IncludeGuardScope::IncludeGuardScope(`
`21`	`21`	`return '_';`
`22`	`22`	`}`
`23`	`23`
`24`		`- return std::toupper(ch);`
	`24`	`+ return static_cast<char>(std::toupper(ch));`
`25`	`25`	`});`
`26`	`26`
`27`	`27`	`_outputFile << R"cpp(// Copyright (c) Microsoft Corporation. All rights reserved.`