Warnings (#281)

* Fixing some warnings

* Make gtest a system library

* Fixing format

* Adding better method for adding warnings

* Nicer Windows deprecated test

* JSON update and drop testing timer

* Warnings as errors everywhere

.appveyor.yml

@@ -13,7 +13,7 @@ install:
 build_script:
   - mkdir build
   - cd build
-  - ps: cmake .. -DCLI11_SINGLE_FILE_TESTS=ON -DCMAKE_BUILD_TYPE=Debug -DCMAKE_GENERATOR="Visual Studio 14 2015"
+  - ps: cmake .. -DCLI11_WARNINGS_AS_ERRORS=ON -DCLI11_SINGLE_FILE_TESTS=ON -DCMAKE_BUILD_TYPE=Debug -DCMAKE_GENERATOR="Visual Studio 14 2015"
   - ps: cmake --build .
   - cd ..
   - conan create . CLIUtils/CLI11

.ci/azure-build.yml

@@ -2,7 +2,7 @@ steps:
 
 - task: CMake@1
   inputs:
-    cmakeArgs: .. -DCLI11_SINGLE_FILE=$(cli11.single) -DCLI11_CXX_STD=$(cli11.std) -DCLI11_SINGLE_FILE_TESTS=$(cli11.single) -DCMAKE_BUILD_TYPE=$(cli11.build_type) $(cli11.options)
+    cmakeArgs: .. -DCLI11_WARNINGS_AS_ERRORS=ON -DCLI11_SINGLE_FILE=$(cli11.single) -DCLI11_CXX_STD=$(cli11.std) -DCLI11_SINGLE_FILE_TESTS=$(cli11.single) -DCMAKE_BUILD_TYPE=$(cli11.build_type) $(cli11.options)
   displayName: 'Configure'
 
 - script: cmake --build .

.ci/make_and_test.sh

@@ -8,7 +8,7 @@ set -evx
 
 mkdir -p build
 cd build
-cmake .. -DCLI11_SINGLE_FILE=ON -DCLI11_CXX_STD=$STD -DCLI11_SINGLE_FILE_TESTS=ON -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_COMPILER_LAUNCHER=ccache $@
+cmake .. -DCLI11_WARNINGS_AS_ERRORS=ON -DCLI11_SINGLE_FILE=ON -DCLI11_CXX_STD=$STD -DCLI11_SINGLE_FILE_TESTS=ON -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_COMPILER_LAUNCHER=ccache $@
 cmake --build . -- -j2
 
 set +evx

CMakeLists.txt

@@ -11,9 +11,6 @@ else()
     cmake_policy(VERSION 3.14)
 endif()
 
-# TESTING: remove this later
-set_property(GLOBAL PROPERTY RULE_LAUNCH_COMPILE "${CMAKE_COMMAND} -E time")
-
 set(VERSION_REGEX "#define CLI11_VERSION[ \t]+\"(.+)\"")
 
 # Read in the line containing the version
@@ -26,6 +23,9 @@ string(REGEX REPLACE ${VERSION_REGEX} "\\1" VERSION_STRING "${VERSION_STRING}")
 # Add the project
 project(CLI11 LANGUAGES CXX VERSION ${VERSION_STRING})
 
+# Special target that adds warnings. Is not exported.
+add_library(CLI11_warnings INTERFACE)
+
 # Only if built as the main project
 if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME)
     # User settable
@@ -45,11 +45,19 @@ if(CMAKE_PROJECT_NAME STREQUAL PROJECT_NAME)
     set(CMAKE_CXX_EXTENSIONS OFF)
     set(CMAKE_CXX_STANDARD_REQUIRED ON)
 
+    option(CLI11_WARNINGS_AS_ERRORS "Turn all warnings into errors (for CI)")
+
     # Be moderately paranoid with flags
     if(MSVC)
-        add_definitions("/W4")
+        target_compile_options(CLI11_warnings INTERFACE "/W4")
+        if(CLI11_WARNINGS_AS_ERRORS)
+            target_compile_options(CLI11_warnings INTERFACE "/WX")
+        endif()
     else()
-        add_definitions(-Wall -Wextra -pedantic -Wshadow)
+        target_compile_options(CLI11_warnings INTERFACE -Wall -Wextra -pedantic -Wshadow)
+        if(CLI11_WARNINGS_AS_ERRORS)
+            target_compile_options(CLI11_warnings INTERFACE -Werror)
+        endif()
     endif()
 
     if(CMAKE_VERSION VERSION_GREATER 3.6)

cmake/AddGoogletest.cmake

@@ -1,4 +1,4 @@
-# 
+#
 #
 # Downloads GTest and provides a helper macro to add tests. Add make check, as well, which
 # gives output on failed tests without having to set an environment variable.
@@ -25,7 +25,7 @@ endif()
 # Target must already exist
 macro(add_gtest TESTNAME)
     target_link_libraries(${TESTNAME} PUBLIC gtest gmock gtest_main)
-    
+
     if(GOOGLE_TEST_INDIVIDUAL)
         if(CMAKE_VERSION VERSION_LESS 3.10)
             gtest_add_tests(TARGET ${TESTNAME}
@@ -36,7 +36,7 @@ macro(add_gtest TESTNAME)
             gtest_discover_tests(${TESTNAME}
                 TEST_PREFIX "${TESTNAME}."
                 PROPERTIES FOLDER "Tests")
-            
+
         endif()
     else()
         add_test(${TESTNAME} ${TESTNAME})
@@ -59,6 +59,13 @@ BUILD_GTEST
 set_target_properties(gtest gtest_main gmock gmock_main
     PROPERTIES FOLDER "Extern")
 
+foreach(TGT IN ITEMS gtest gtest_main gmock gmock_main)
+    get_property(DIR_LIST TARGET ${TGT} PROPERTY INTERFACE_INCLUDE_DIRECTORIES)
+    foreach(ITEM IN LISTS DIR_LIST)
+        set_property(TARGET ${TGT} APPEND PROPERTY INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "${ITEM}")
+    endforeach()
+endforeach()
+
 if(MSVC)
     if (MSVC_VERSION GREATER_EQUAL 1900)
         target_compile_definitions(gtest PUBLIC _SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING)

examples/CMakeLists.txt

@@ -19,8 +19,8 @@ if(CLI11_EXAMPLE_JSON)
     if(NOT EXISTS "${CLI11_SOURCE_DIR}/extern/json/single_include/nlohmann/json.hpp")
         message(ERROR "You are missing the json package for CLI11_EXAMPLE_JSON. Please update your submodules (git submodule update --init)")
     endif()
-    if(CMAKE_COMPILER_ID STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 4.9)
-        message(WARNING "The json example requires GCC 4.9+ (requirement on json library)")
+    if(CMAKE_COMPILER_ID STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 4.8)
+        message(WARNING "The json example requires GCC 4.8+ (requirement on json library)")
     endif()
     add_cli_exe(json json.cpp)
     target_include_directories(json PUBLIC SYSTEM "${CLI11_SOURCE_DIR}/extern/json/single_include")
@@ -69,7 +69,7 @@ set_property(TEST subcom_partitioned_none PROPERTY PASS_REGULAR_EXPRESSION
     "This is a timer:"
     "--file is required"
     "Run with --help for more information.")
-	
+
 add_test(NAME subcom_partitioned_all COMMAND subcom_partitioned --file this --count --count -d 1.2)
 set_property(TEST subcom_partitioned_all PROPERTY PASS_REGULAR_EXPRESSION
     "This is a timer:"
@@ -93,7 +93,7 @@ set_property(TEST option_groups_extra PROPERTY PASS_REGULAR_EXPRESSION
 add_test(NAME option_groups_extra2 COMMAND option_groups --csv --address "192.168.1.1" -o "test.out")
 set_property(TEST option_groups_extra2 PROPERTY PASS_REGULAR_EXPRESSION
     "at most 1")
-	
+
 add_cli_exe(positional_arity positional_arity.cpp)
 add_test(NAME positional_arity1 COMMAND positional_arity one )
 set_property(TEST positional_arity1 PROPERTY PASS_REGULAR_EXPRESSION
@@ -132,7 +132,7 @@ set_property(TEST shapes_all PROPERTY PASS_REGULAR_EXPRESSION
     "circle4"
     "rectangle2 with edges [2.1,2.1]"
     "triangel1 with sides [4.5]")
-	
+
 add_cli_exe(ranges ranges.cpp)
 add_test(NAME ranges_range COMMAND ranges --range 1 2 3)
 set_property(TEST ranges_range PROPERTY PASS_REGULAR_EXPRESSION

include/CLI/App.hpp

@@ -1105,8 +1105,11 @@ class App {
     }
     /// Get a pointer to subcommand by index
     App *get_subcommand(int index = 0) const {
-        if((index >= 0) && (index < static_cast<int>(subcommands_.size())))
-            return subcommands_[index].get();
+        if(index >= 0) {
+            auto uindex = static_cast<unsigned>(index);
+            if(uindex < subcommands_.size())
+                return subcommands_[uindex].get();
+        }
         throw OptionNotFound(std::to_string(index));
     }
 
@@ -1130,8 +1133,11 @@ class App {
 
     /// Get an owning pointer to subcommand by index
     CLI::App_p get_subcommand_ptr(int index = 0) const {
-        if((index >= 0) && (index < static_cast<int>(subcommands_.size())))
-            return subcommands_[index];
+        if(index >= 0) {
+            auto uindex = static_cast<unsigned>(index);
+            if(uindex < subcommands_.size())
+                return subcommands_[uindex];
+        }
         throw OptionNotFound(std::to_string(index));
     }
 
@@ -1274,13 +1280,13 @@ class App {
     /// This must be called after the options are in but before the rest of the program.
     void parse(int argc, const char *const *argv) {
         // If the name is not set, read from command line
-        if((name_.empty()) || (has_automatic_name_)) {
+        if(name_.empty() || has_automatic_name_) {
             has_automatic_name_ = true;
             name_ = argv[0];
         }
 
         std::vector<std::string> args;
-        args.reserve(argc - 1);
+        args.reserve(static_cast<size_t>(argc - 1));
         for(int i = argc - 1; i > 0; i--)
             args.emplace_back(argv[i]);
         parse(std::move(args));

include/CLI/Option.hpp

@@ -722,7 +722,7 @@ class Option : public OptionBase<Option> {
             } else if(get_items_expected() < 0) {
                 // Require that this be a multiple of expected size and at least as many as expected
                 if(results_.size() < static_cast<size_t>(-get_items_expected()) ||
-                   results_.size() % static_cast<size_t>(std::abs(get_type_size())) != 0)
+                   results_.size() % static_cast<size_t>(std::abs(get_type_size())) != 0u)
                     throw ArgumentMismatch(get_name(), get_items_expected(), results_.size());
             }
             local_result = !callback_(results_);
@@ -799,7 +799,8 @@ class Option : public OptionBase<Option> {
             if(!((input_value.empty()) || (input_value == emptyString))) {
                 auto default_ind = detail::find_member(name, fnames_, ignore_case_, ignore_underscore_);
                 if(default_ind >= 0) {
-                    if(default_flag_values_[default_ind].second != input_value) {
+                    // We can static cast this to size_t because it is more than 0 in this block
+                    if(default_flag_values_[static_cast<size_t>(default_ind)].second != input_value) {
                         throw(ArgumentMismatch::FlagOverride(name));
                     }
                 } else {
@@ -811,12 +812,12 @@ class Option : public OptionBase<Option> {
         }
         auto ind = detail::find_member(name, fnames_, ignore_case_, ignore_underscore_);
         if((input_value.empty()) || (input_value == emptyString)) {
-            return (ind < 0) ? trueString : default_flag_values_[ind].second;
+            return (ind < 0) ? trueString : default_flag_values_[static_cast<size_t>(ind)].second;
         }
         if(ind < 0) {
             return input_value;
         }
-        if(default_flag_values_[ind].second == falseString) {
+        if(default_flag_values_[static_cast<size_t>(ind)].second == falseString) {
             try {
                 auto val = detail::to_flag_value(input_value);
                 return (val == 1) ? falseString : (val == (-1) ? trueString : std::to_string(-val));

include/CLI/StringTools.hpp

@@ -231,11 +231,12 @@ inline bool has_default_flag_values(const std::string &flags) {
 }
 
 inline void remove_default_flag_values(std::string &flags) {
-    size_t loc = flags.find_first_of('{');
+    auto loc = flags.find_first_of('{');
     while(loc != std::string::npos) {
         auto finish = flags.find_first_of("},", loc + 1);
         if((finish != std::string::npos) && (flags[finish] == '}')) {
-            flags.erase(flags.begin() + loc, flags.begin() + finish + 1);
+            flags.erase(flags.begin() + static_cast<std::ptrdiff_t>(loc),
+                        flags.begin() + static_cast<std::ptrdiff_t>(finish) + 1);
         }
         loc = flags.find_first_of('{', loc + 1);
     }

include/CLI/Timer.hpp

@@ -68,9 +68,9 @@ class Timer {
             f();
             std::chrono::duration<double> elapsed = clock::now() - start_;
             total_time = elapsed.count();
-        } while(n++ < 100 && total_time < target_time);
+        } while(n++ < 100u && total_time < target_time);
 
-        std::string out = make_time_str(total_time / n) + " for " + std::to_string(n) + " tries";
+        std::string out = make_time_str(total_time / static_cast<double>(n)) + " for " + std::to_string(n) + " tries";
         start_ = start;
         return out;
     }
@@ -79,7 +79,7 @@ class Timer {
     std::string make_time_str() const {
         time_point stop = clock::now();
         std::chrono::duration<double> elapsed = stop - start_;
-        double time = elapsed.count() / cycles;
+        double time = elapsed.count() / static_cast<double>(cycles);
         return make_time_str(time);
     }
 

include/CLI/Validators.hpp

@@ -800,7 +800,7 @@ class AsNumberWithUnit : public Validator {
             }
 
             std::string unit{unit_begin, input.end()};
-            input.resize(std::distance(input.begin(), unit_begin));
+            input.resize(static_cast<size_t>(std::distance(input.begin(), unit_begin)));
             detail::trim(input);
 
             if(opts & UNIT_REQUIRED && unit.empty()) {

tests/CMakeLists.txt

@@ -55,7 +55,7 @@ foreach(T ${CLI11_TESTS})
 
     add_executable(${T} ${T}.cpp ${CLI11_headers})
     add_sanitizers(${T})
-    target_link_libraries(${T} PUBLIC CLI11)
+    target_link_libraries(${T} PUBLIC CLI11 CLI11_warnings)
     add_gtest(${T})
 
     if(CLI11_SINGLE_FILE AND CLI11_SINGLE_FILE_TESTS)
@@ -84,6 +84,11 @@ if(NOT MSVC)
     if(TARGET DeprecatedTest_Single)
         target_compile_options(DeprecatedTest_Single PRIVATE -Wno-deprecated-declarations)
     endif()
+else()
+    target_compile_options(DeprecatedTest PRIVATE "/wd4996")
+    if(TARGET DeprecatedTest_Single)
+        target_compile_options(DeprecatedTest_Single PRIVATE "/wd4996")
+    endif()
 endif()
 
 # Link test (build error if inlines missing)

tests/HelpersTest.cpp

@@ -86,7 +86,7 @@ TEST(StringTools, Modify3) {
     std::string newString = CLI::detail::find_and_modify("baaaaaaaaaa", "aaa", [](std::string &str, size_t index) {
         str.erase(index, 3);
         str.insert(str.begin(), 'a');
-        return 0;
+        return 0u;
     });
     EXPECT_EQ(newString, "aba");
 }

tests/TransformTest.cpp

@@ -612,7 +612,7 @@ TEST_F(TApp, NumberWithUnitIntOverflow) {
 }
 
 TEST_F(TApp, NumberWithUnitFloatOverflow) {
-    std::map<std::string, float> mapping{{"a", 2}, {"b", 1}, {"c", 0}};
+    std::map<std::string, float> mapping{{"a", 2.f}, {"b", 1.f}, {"c", 0.f}};
 
     float value;
     app.add_option("-n", value)->transform(CLI::AsNumberWithUnit(mapping));
@@ -622,11 +622,11 @@ TEST_F(TApp, NumberWithUnitFloatOverflow) {
 
     args = {"-n", "3e+38 b"};
     run();
-    EXPECT_FLOAT_EQ(value, 3e+38);
+    EXPECT_FLOAT_EQ(value, 3e+38f);
 
     args = {"-n", "3e+38 c"};
     run();
-    EXPECT_FLOAT_EQ(value, 0);
+    EXPECT_FLOAT_EQ(value, 0.f);
 }
 
 TEST_F(TApp, AsSizeValue1000_1024) {
@@ -639,7 +639,7 @@ TEST_F(TApp, AsSizeValue1000_1024) {
 
     args = {"-s", "1b"};
     run();
-    EXPECT_FLOAT_EQ(value, 1);
+    EXPECT_EQ(value, 1u);
 
     uint64_t k_value = 1000u;
     uint64_t ki_value = 1024u;
@@ -745,7 +745,7 @@ TEST_F(TApp, AsSizeValue1024) {
 
     args = {"-s", "1b"};
     run();
-    EXPECT_FLOAT_EQ(value, 1);
+    EXPECT_EQ(value, 1u);
 
     uint64_t ki_value = 1024u;
     args = {"-s", "1k"};