meta: treat compiler warnings as errors in CI

.github/workflows/cmake.yml

@@ -27,7 +27,7 @@ jobs:
       run: sudo apt-get update -qy
         && sudo apt-get install gcovr -qy
     - name: configure
-      run: cmake -DBUILD_TESTING=on -DPATCH_ENABLE_COVERAGE=yes -S . -B build
+      run: cmake -DBUILD_TESTING=on -DPATCH_ENABLE_COVERAGE=yes -DPATCH_WARNINGS_AS_ERRORS=on -S . -B build
     - name: compile
       run: cmake --build build -j2
     - name: test
@@ -49,7 +49,7 @@ jobs:
     steps:
     - uses: actions/checkout@v6
     - name: configure
-      run: cmake -DBUILD_TESTING=on -S . -B build
+      run: cmake -DBUILD_TESTING=on -DPATCH_WARNINGS_AS_ERRORS=on -S . -B build
     - name: compile
       run: cmake --build build -j2
     - name: test
@@ -73,7 +73,7 @@ jobs:
           mingw-w64-ucrt-x86_64-cmake
           mingw-w64-ucrt-x86_64-ninja
     - name: configure
-      run: cmake -G Ninja -DBUILD_TESTING=on -S . -B build
+      run: cmake -G Ninja -DBUILD_TESTING=on -DPATCH_WARNINGS_AS_ERRORS=on -S . -B build
     - name: compile
       run: cmake --build build -j2
     - name: test

.github/workflows/msvc.yml

@@ -26,7 +26,7 @@ jobs:
         uses: actions/checkout@v6
 
       - name: Configure CMake
-        run: cmake -B ${{ env.build }} -DCMAKE_BUILD_TYPE=${{ env.config }}
+        run: cmake -B ${{ env.build }} -DCMAKE_BUILD_TYPE=${{ env.config }} -DPATCH_WARNINGS_AS_ERRORS=on
 
       - name: Initialize MSVC Code Analysis
         uses: microsoft/msvc-code-analysis-action@v0.1.1

CMakeLists.txt

@@ -22,8 +22,41 @@ include(GNUInstallDirs)
 include(coverage)
 
 option(PATCH_ENABLE_COVERAGE "Build with gcov support" OFF)
+option(PATCH_WARNINGS_AS_ERRORS "Enable strict compiler warnings and treat them as errors" OFF)
 option(BUILD_TESTING "Build the tests" OFF)
 
+function(patch_configure_target target_name)
+  if(WIN32)
+    target_compile_definitions(${target_name} PRIVATE
+      NOMINMAX
+      WIN32_LEAN_AND_MEAN
+    )
+  endif()
+
+  if(NOT PATCH_WARNINGS_AS_ERRORS)
+    return()
+  endif()
+
+  if(MSVC)
+    target_compile_options(${target_name} PRIVATE /W4 /WX)
+    target_compile_definitions(${target_name} PRIVATE
+      _CRT_NONSTDC_NO_WARNINGS
+      _CRT_SECURE_NO_WARNINGS
+    )
+  elseif(CMAKE_CXX_COMPILER_ID MATCHES "^(AppleClang|Clang|GNU)$")
+    target_compile_options(${target_name} PRIVATE
+      -Wall
+      -Wextra
+      -Wpedantic
+      -Wconversion
+      -Wsign-conversion
+      -Werror
+    )
+  else()
+    message(FATAL_ERROR "PATCH_WARNINGS_AS_ERRORS does not support ${CMAKE_CXX_COMPILER_ID}")
+  endif()
+endfunction()
+
 if(PATCH_ENABLE_COVERAGE)
   add_coverage_flags()
 endif()
@@ -39,6 +72,7 @@ add_library(patch
   src/system.cpp
   src/file.cpp
 )
+patch_configure_target(patch)
 
 target_compile_features(patch PUBLIC cxx_std_11)
 

app/CMakeLists.txt

@@ -2,6 +2,7 @@
 # Copyright 2022 Shannon Booth <shannon.ml.booth@gmail.com>
 
 add_executable(sb_patch main.cpp)
+patch_configure_target(sb_patch)
 target_link_libraries(sb_patch PRIVATE patch::patch)
 
 install(TARGETS sb_patch

include/patch/applier.h

@@ -24,19 +24,19 @@ class RejectWriter {
 
     void write_reject_file(const Hunk& hunk);
 
-    int rejected_hunks() const { return m_rejected_hunks; }
+    size_t rejected_hunks() const { return m_rejected_hunks; }
 
 private:
     bool should_write_as_unified() const;
 
     const Patch& m_patch;
-    int m_rejected_hunks { 0 };
+    size_t m_rejected_hunks { 0 };
     File& m_reject_file;
     Options::RejectFormat m_reject_format { Options::RejectFormat::Default };
 };
 
 struct Result {
-    int failed_hunks;
+    size_t failed_hunks;
     bool was_skipped;
     bool all_hunks_applied_perfectly;
 };

src/cmdline.cpp

@@ -2,6 +2,7 @@
 // Copyright 2022-2023 Shannon Booth <shannon.ml.booth@gmail.com>
 
 #include <algorithm>
+#include <cstddef>
 #include <cstring>
 #include <patch/cmdline.h>
 #include <patch/system.h>
@@ -11,6 +12,9 @@
 
 #ifdef _WIN32
 #    include <windows.h>
+
+// shellapi.h depends on types and macros declared by windows.h.
+#    include <shellapi.h>
 #endif
 
 namespace Patch {
@@ -29,8 +33,9 @@ CmdLine::CmdLine(int argc, const char* const* argv)
     if (!wide_argv)
         throw std::bad_alloc();
 
-    narrowed_argv_str.reserve(m_argc);
-    narrowed_argv.reserve(m_argc);
+    const auto argument_count = static_cast<std::size_t>(m_argc);
+    narrowed_argv_str.reserve(argument_count);
+    narrowed_argv.reserve(argument_count);
 
     for (int i = 0; i < m_argc; ++i) {
         narrowed_argv_str.emplace_back(to_narrow(wide_argv[i]));

src/parser.cpp

@@ -124,20 +124,23 @@ std::string LineParser::parse_quoted_string()
             case '5':
             case '6':
             case '7': {
-                unsigned char result = static_cast<unsigned char>(c) - '0';
+                unsigned int result = static_cast<unsigned int>(c - '0');
 
                 for (int i = 1; i < 3; ++i) {
                     char octal_val = peek();
                     if (!is_octal(octal_val))
                         break;
 
-                    unsigned char digit_val = static_cast<unsigned char>(octal_val) - '0';
-                    result = result * 8 + digit_val;
+                    unsigned int digit_val = static_cast<unsigned int>(octal_val - '0');
+                    result = result * 8U + digit_val;
 
                     ++m_current;
                 }
 
-                output += static_cast<char>(result);
+                if (result > static_cast<unsigned int>(std::numeric_limits<unsigned char>::max()))
+                    throw std::invalid_argument("Octal escape is out of range in path " + std::string(begin, m_current));
+
+                output += static_cast<char>(static_cast<unsigned char>(result));
                 break;
             }
             default:
@@ -807,7 +810,7 @@ void Parser::parse_context_hunk(std::vector<PatchLine>& old_lines, LineNumber& o
 {
     std::string line;
 
-    LineNumber from_file_range_line_number = 0;
+    size_t from_file_range_line_number = 0;
 
     LineNumber old_end_line = 0;
     LineNumber new_end_line = 0;

src/system.cpp

@@ -20,9 +20,9 @@
 #include <utility>
 
 #ifdef _WIN32
+#    include "windows_error.h"
 #    include <direct.h>
 #    include <io.h>
-#    include <windows.h>
 #    define close _close
 #    define read _read
 #    define open _open
@@ -79,7 +79,14 @@ std::string read_tty_until_enter()
     size_t offset = 0;
 
     while (true) {
-        auto ret = ::read(fd, &buffer[0] + offset, buffer.size() - offset);
+        const auto available_size = buffer.size() - offset;
+#ifdef _WIN32
+        const auto read_size = std::min(available_size, static_cast<size_t>(INT_MAX));
+        auto ret = ::read(fd, &buffer[0] + offset, static_cast<unsigned int>(read_size));
+#else
+        const auto read_size = available_size;
+        auto ret = ::read(fd, &buffer[0] + offset, read_size);
+#endif
         if (ret < 0) {
             int saved_errno = errno;
             ::close(fd);
@@ -88,7 +95,7 @@ std::string read_tty_until_enter()
 
         // Finish if we didn't read up until the end of our buffer, indicating input has finished, or
         // if the last character given was an enter which means that the user has submitted their answer.
-        if (buffer.size() - offset != static_cast<size_t>(ret) || buffer.back() == '\n') {
+        if (read_size != static_cast<size_t>(ret) || buffer.back() == '\n') {
             // Trim to size, any pop any trailing '\n' since that is not part of their answer.
             buffer.resize(offset + static_cast<size_t>(ret));
             if (!buffer.empty() && buffer.back() == '\n')
@@ -187,7 +194,7 @@ std::string current_path()
         const auto size = GetCurrentDirectoryW(requested_size, &result[0]);
 
         if (size == 0)
-            throw std::system_error(GetLastError(), std::system_category(), "Failed getting current directory");
+            throw last_win32_error("Failed getting current directory");
 
         result.resize(size);
         if (size <= requested_size)
@@ -310,7 +317,7 @@ std::string temp_directory_path()
         const auto size = GetTempPathW(requested_size, &result[0]);
 
         if (size == 0)
-            throw std::system_error(GetLastError(), std::system_category(), "Failed getting current directory");
+            throw last_win32_error("Failed getting current directory");
 
         result.resize(size);
         if (size <= requested_size)
@@ -372,7 +379,7 @@ void symlink(const std::string& target, const std::string& linkpath)
         error = GetLastError();
     }
 
-    throw std::system_error(error, std::system_category(), "Can't create symbolic link " + target + " ");
+    throw win32_error(error, "Can't create symbolic link " + target + " ");
 #else
     int ret = ::symlink(target.c_str(), linkpath.c_str());
     if (ret != 0)
@@ -511,19 +518,23 @@ void rename(const std::string& old_path, const std::string& new_path)
         const DWORD attributes = GetFileAttributesW(native_new_path.c_str());
         if (attributes != INVALID_FILE_ATTRIBUTES && (attributes & FILE_ATTRIBUTE_READONLY)) {
             const DWORD writable_attributes = attributes & ~static_cast<DWORD>(FILE_ATTRIBUTE_READONLY);
-            if (SetFileAttributesW(native_new_path.c_str(), writable_attributes) == 0)
-                throw std::system_error(GetLastError(), std::system_category(), "Unable to make file writable " + new_path);
+            if (SetFileAttributesW(native_new_path.c_str(), writable_attributes) == 0) {
+                const auto attributes_error = GetLastError();
+                throw win32_error(attributes_error, "Unable to make file writable " + new_path);
+            }
 
             if (MoveFileExW(native_old_path.c_str(), native_new_path.c_str(), MOVEFILE_REPLACE_EXISTING) != 0)
                 return;
 
             error = GetLastError();
-            if (SetFileAttributesW(native_new_path.c_str(), attributes) == 0)
-                throw std::system_error(GetLastError(), std::system_category(), "Unable to restore permissions to " + new_path);
+            if (SetFileAttributesW(native_new_path.c_str(), attributes) == 0) {
+                const auto attributes_error = GetLastError();
+                throw win32_error(attributes_error, "Unable to restore permissions to " + new_path);
+            }
         }
     }
 
-    throw std::system_error(error, std::system_category(), "Unable to rename " + old_path + " to " + new_path);
+    throw win32_error(error, "Unable to rename " + old_path + " to " + new_path);
 #else
     if (std::rename(old_path.c_str(), new_path.c_str()) != 0)
         throw std::system_error(errno, std::generic_category(), "Unable to rename " + old_path + " to " + new_path);
@@ -539,8 +550,10 @@ void permissions(const std::string& path, perms permissions)
     const auto native = to_native(path);
 
     DWORD attributes = GetFileAttributesW(native.c_str());
-    if (attributes == INVALID_FILE_ATTRIBUTES)
-        throw std::system_error(GetLastError(), std::system_category(), "Unable to set permissions to " + path);
+    if (attributes == INVALID_FILE_ATTRIBUTES) {
+        const auto attributes_error = GetLastError();
+        throw win32_error(attributes_error, "Unable to set permissions to " + path);
+    }
 
     // No group/owner/all on Windows - if any are set treat as write permissions.
     const auto write_perms = perms::owner_write | perms::group_write | perms::others_write;
@@ -554,10 +567,12 @@ void permissions(const std::string& path, perms permissions)
     if (should_be_read_only)
         attributes |= FILE_ATTRIBUTE_READONLY;
     else
-        attributes &= ~FILE_ATTRIBUTE_READONLY;
+        attributes &= ~static_cast<DWORD>(FILE_ATTRIBUTE_READONLY);
 
-    if (SetFileAttributesW(native.c_str(), attributes) == 0)
-        throw std::system_error(GetLastError(), std::system_category(), "Unable to set permissions to " + path);
+    if (SetFileAttributesW(native.c_str(), attributes) == 0) {
+        const auto attributes_error = GetLastError();
+        throw win32_error(attributes_error, "Unable to set permissions to " + path);
+    }
 
 #else
     if (::chmod(path.c_str(), static_cast<mode_t>(permissions)) != 0)
@@ -600,7 +615,7 @@ void permissions(FILE* file, perms permissions)
 
     FILE_BASIC_INFO info;
     if (GetFileInformationByHandleEx(handle, FileBasicInfo, &info, sizeof(info)) == 0)
-        throw std::system_error(GetLastError(), std::system_category(), "Unable to change permissions");
+        throw last_win32_error("Unable to change permissions");
 
     const auto write_permissions = perms::owner_write | perms::group_write | perms::others_write;
     if ((permissions & write_permissions) == perms::none)
@@ -609,7 +624,7 @@ void permissions(FILE* file, perms permissions)
         info.FileAttributes &= ~static_cast<DWORD>(FILE_ATTRIBUTE_READONLY);
 
     if (SetFileInformationByHandle(handle, FileBasicInfo, &info, sizeof(info)) == 0)
-        throw std::system_error(GetLastError(), std::system_category(), "Unable to change permissions");
+        throw last_win32_error("Unable to change permissions");
 #else
     if (::fchmod(fileno(file), static_cast<mode_t>(permissions)) != 0)
         throw std::system_error(errno, std::generic_category(), "Unable to change permissions");
@@ -625,7 +640,7 @@ perms get_permissions(FILE* file)
 
     FILE_BASIC_INFO info;
     if (GetFileInformationByHandleEx(handle, FileBasicInfo, &info, sizeof(info)) == 0)
-        throw std::system_error(GetLastError(), std::system_category(), "Unable to get permissions");
+        throw last_win32_error("Unable to get permissions");
 
     perms permissions = perms::owner_read | perms::group_read | perms::others_read;
     if (!(info.FileAttributes & FILE_ATTRIBUTE_READONLY))
@@ -647,7 +662,10 @@ uintmax_t file_size(FILE* file)
     if (fstat(fileno(file), &buf) != 0)
         throw std::system_error(errno, std::generic_category(), "Unable to fstat file");
 
-    return buf.st_size;
+    if (buf.st_size < 0)
+        throw std::system_error(std::make_error_code(std::errc::invalid_argument), "File has a negative size");
+
+    return static_cast<uintmax_t>(buf.st_size);
 }
 
 } // namespace filesystem
@@ -661,14 +679,14 @@ std::wstring to_wide(const std::string& str)
 
     int length = MultiByteToWideChar(CP_UTF8, 0, str.data(), static_cast<int>(str.size()), nullptr, 0);
     if (length == 0)
-        throw std::system_error(GetLastError(), std::system_category(), "Failed widening string");
+        throw last_win32_error("Failed widening string");
 
     std::wstring wide_str;
     wide_str.resize(static_cast<size_t>(length));
 
     length = MultiByteToWideChar(CP_UTF8, 0, str.data(), static_cast<int>(str.size()), &wide_str[0], length);
     if (length == 0)
-        throw std::system_error(GetLastError(), std::system_category(), "Failed widening string");
+        throw last_win32_error("Failed widening string");
 
     return wide_str;
 }
@@ -680,14 +698,14 @@ std::string to_narrow(const std::wstring& str)
 
     int length = WideCharToMultiByte(CP_UTF8, 0, str.c_str(), static_cast<int>(str.size()), nullptr, 0, nullptr, nullptr);
     if (length == 0)
-        throw std::system_error(GetLastError(), std::system_category(), "Failed narrowing string");
+        throw last_win32_error("Failed narrowing string");
 
     std::string narrow_str;
     narrow_str.resize(static_cast<size_t>(length));
 
     length = WideCharToMultiByte(CP_UTF8, 0, str.c_str(), static_cast<int>(str.size()), &narrow_str[0], length, nullptr, nullptr);
     if (length == 0)
-        throw std::system_error(GetLastError(), std::system_category(), "Failed narrowing string");
+        throw last_win32_error("Failed narrowing string");
 
     return narrow_str;
 }