From 1cc365c0f963d260f5703519b0e68f0b0d14957e Mon Sep 17 00:00:00 2001 From: Scott Cyphers Date: Wed, 25 Sep 2019 08:31:08 -0700 Subject: [PATCH] Cyphers/lastwarns (#3640) * Build changes for -Wall * Use properties * Remove no-zero-as-null-pointer-constant * add ##__VAR_ARGS__ workaround * enable gnu-zero-variadic-macro-arguments warning * fix gnu-zero-variadic-macro-arguments * use PrintToDummyParamName * remove ##__VA_ARGS__ workaround * fix ##__VA_ARGS__ and enable gnu-zero-variadic-macro-arguments * handle windows build * use alternative fix to support VS compiler --- CMakeLists.txt | 2 + cmake/clang_4_0_flags.cmake | 28 +++--- cmake/external_onnx.cmake | 6 +- cmake/external_protobuf.cmake | 5 +- src/ngraph/check.hpp | 87 +++++++++++++++++-- .../frontend/onnx_import/CMakeLists.txt | 5 +- src/ngraph/node.hpp | 4 +- test/check.cpp | 2 +- test/type_prop/dyn_replace_slice.cpp | 3 +- test/type_prop/dyn_slice.cpp | 3 +- test/type_prop/range.cpp | 9 +- test/util/type_prop.hpp | 9 ++ 12 files changed, 127 insertions(+), 36 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 6e4bac43639..0a6b368d471 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -370,6 +370,8 @@ if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU") endif() endif() +set(CMAKE_ORIGINAL_CXX_FLAGS "${CMAKE_CXX_FLAGS}") + ngraph_var(NGRAPH_WARNINGS_AS_ERRORS DEFAULT "OFF") if (${NGRAPH_WARNINGS_AS_ERRORS}) set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror") diff --git a/cmake/clang_4_0_flags.cmake b/cmake/clang_4_0_flags.cmake index 8adf453b85e..93f37a70311 100644 --- a/cmake/clang_4_0_flags.cmake +++ b/cmake/clang_4_0_flags.cmake @@ -20,23 +20,19 @@ add_compile_options(-Werror=comment) add_compile_options(-pedantic-errors) # whitelist errors here -add_compile_options(-Weverything) -add_compile_options(-Wno-gnu-zero-variadic-macro-arguments) -add_compile_options(-Wno-c++98-compat-pedantic) -add_compile_options(-Wno-weak-vtables) -add_compile_options(-Wno-global-constructors) -add_compile_options(-Wno-exit-time-destructors) -add_compile_options(-Wno-missing-prototypes) -add_compile_options(-Wno-missing-noreturn) -add_compile_options(-Wno-covered-switch-default) -add_compile_options(-Wno-undef) +#add_compile_options(-Weverything) +add_compile_options(-Wall) +# add_compile_options(-Wno-gnu-zero-variadic-macro-arguments) +#add_compile_options(-Wno-c++98-compat-pedantic) +#add_compile_options(-Wno-weak-vtables) +#add_compile_options(-Wno-global-constructors) +#add_compile_options(-Wno-exit-time-destructors) +#add_compile_options(-Wno-missing-prototypes) +#add_compile_options(-Wno-missing-noreturn) +#add_compile_options(-Wno-covered-switch-default) +#add_compile_options(-Wno-undef) if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "AppleClang") if (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 9.1.0) - add_compile_options(-Wno-zero-as-null-pointer-constant) + #add_compile_options(-Wno-zero-as-null-pointer-constant) endif() endif() - -# should remove these -add_compile_options(-Wno-padded) -add_compile_options(-Wno-conversion) -add_compile_options(-Wno-double-promotion) diff --git a/cmake/external_onnx.cmake b/cmake/external_onnx.cmake index b115251c441..83c57089ab9 100644 --- a/cmake/external_onnx.cmake +++ b/cmake/external_onnx.cmake @@ -44,7 +44,7 @@ ExternalProject_Add( CMAKE_GENERATOR_PLATFORM ${CMAKE_GENERATOR_PLATFORM} CMAKE_GENERATOR_TOOLSET ${CMAKE_GENERATOR_TOOLSET} CMAKE_ARGS ${NGRAPH_FORWARD_CMAKE_ARGS} - -DCMAKE_CXX_FLAGS=${CMAKE_CXX_FLAGS} + -DCMAKE_CXX_FLAGS=${CMAKE_ORIGINAL_CXX_FLAGS} -DONNX_GEN_PB_TYPE_STUBS=OFF -DCMAKE_PREFIX_PATH=${Protobuf_INSTALL_PREFIX} -DONNX_ML=TRUE @@ -85,7 +85,7 @@ set(ONNX_LIBRARIES ${ONNX_LIBRARY} ${ONNX_PROTO_LIBRARY}) if (NOT TARGET onnx::libonnx) add_library(onnx::libonnx UNKNOWN IMPORTED) set_target_properties(onnx::libonnx PROPERTIES - INTERFACE_INCLUDE_DIRECTORIES ${ONNX_INCLUDE_DIR} + INTERFACE_SYSTEM_INCLUDE_DIRECTORIES ${ONNX_INCLUDE_DIR} IMPORTED_LOCATION ${ONNX_LIBRARY}) add_dependencies(onnx::libonnx ext_onnx) endif() @@ -93,7 +93,7 @@ endif() if (NOT TARGET onnx::libonnx_proto) add_library(onnx::libonnx_proto UNKNOWN IMPORTED) set_target_properties(onnx::libonnx_proto PROPERTIES - INTERFACE_INCLUDE_DIRECTORIES ${ONNX_PROTO_INCLUDE_DIR} + INTERFACE_SYSTEM_INCLUDE_DIRECTORIES ${ONNX_PROTO_INCLUDE_DIR} IMPORTED_LOCATION ${ONNX_PROTO_LIBRARY}) add_dependencies(onnx::libonnx_proto ext_onnx) endif() diff --git a/cmake/external_protobuf.cmake b/cmake/external_protobuf.cmake index 4d2386abf9f..7ef9e5f66ad 100644 --- a/cmake/external_protobuf.cmake +++ b/cmake/external_protobuf.cmake @@ -38,7 +38,7 @@ if (WIN32) CMAKE_GENERATOR_TOOLSET ${CMAKE_GENERATOR_TOOLSET} CMAKE_ARGS ${NGRAPH_FORWARD_CMAKE_ARGS} - -DCMAKE_CXX_FLAGS=${CMAKE_CXX_FLAGS} + -DCMAKE_CXX_FLAGS=${CMAKE_ORIGINAL_CXX_FLAGS} -Dprotobuf_MSVC_STATIC_RUNTIME=OFF -Dprotobuf_WITH_ZLIB=OFF -Dprotobuf_BUILD_TESTS=OFF @@ -108,7 +108,7 @@ set(Protobuf_LIBRARIES ${Protobuf_LIBRARY}) if (NOT TARGET protobuf::libprotobuf) add_library(protobuf::libprotobuf UNKNOWN IMPORTED) set_target_properties(protobuf::libprotobuf PROPERTIES - INTERFACE_INCLUDE_DIRECTORIES "${Protobuf_INCLUDE_DIR}" + INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "${Protobuf_INCLUDE_DIR}" IMPORTED_LOCATION "${Protobuf_LIBRARY}") add_dependencies(protobuf::libprotobuf ext_protobuf) endif() @@ -116,6 +116,7 @@ endif() if (NOT TARGET protobuf::protoc) add_executable(protobuf::protoc IMPORTED) set_target_properties(protobuf::protoc PROPERTIES + INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "${Protobuf_PROTOC_EXECUTABLE}" IMPORTED_LOCATION "${Protobuf_PROTOC_EXECUTABLE}") add_dependencies(protobuf::protoc ext_protobuf) endif() diff --git a/src/ngraph/check.hpp b/src/ngraph/check.hpp index 8c301f40bea..efcce9143c0 100644 --- a/src/ngraph/check.hpp +++ b/src/ngraph/check.hpp @@ -136,29 +136,106 @@ namespace ngraph // TODO(amprocte): refactor NGRAPH_CHECK_HELPER so we don't have to introduce a locally-scoped // variable (ss___) and risk shadowing. // -#define NGRAPH_CHECK_HELPER(exc_class, ctx, check, ...) \ +#define NGRAPH_CHECK_HELPER2(exc_class, ctx, check, ...) \ do \ { \ if (!(check)) \ { \ ::std::stringstream ss___; \ - ::ngraph::write_all_to_stream(ss___, ##__VA_ARGS__); \ + ::ngraph::write_all_to_stream(ss___, __VA_ARGS__); \ throw exc_class( \ (::ngraph::CheckLocInfo{__FILE__, __LINE__, #check}), (ctx), ss___.str()); \ } \ } while (0) +#define NGRAPH_CHECK_HELPER1(exc_class, ctx, check) \ + do \ + { \ + if (!(check)) \ + { \ + throw exc_class((::ngraph::CheckLocInfo{__FILE__, __LINE__, #check}), (ctx), ""); \ + } \ + } while (0) + /// \brief Macro to check whether a boolean condition holds. /// \param cond Condition to check /// \param ... Additional error message info to be added to the error message via the `<<` /// stream-insertion operator. Note that the expressions here will be evaluated lazily, /// i.e., only if the `cond` evalutes to `false`. /// \throws ::ngraph::CheckFailure if `cond` is false. -#define NGRAPH_CHECK(cond, ...) \ - NGRAPH_CHECK_HELPER(::ngraph::CheckFailure, "", (cond), ##__VA_ARGS__) +#define NGRAPH_CHECK(...) NGRAPH_CHECK_HELPER(::ngraph::CheckFailure, "", __VA_ARGS__) /// \brief Macro to signal a code path that is unreachable in a successful execution. It's /// implemented with NGRAPH_CHECK macro. /// \param ... Additional error message that should describe why that execution path is unreachable. /// \throws ::ngraph::CheckFailure if the macro is executed. -#define NGRAPH_UNREACHABLE(...) NGRAPH_CHECK(false, "Unreachable: ", ##__VA_ARGS__) +#define NGRAPH_UNREACHABLE(...) NGRAPH_CHECK(false, "Unreachable: ", __VA_ARGS__) +#define NGRAPH_CHECK_HELPER(exc_class, ctx, ...) \ + CALL_OVERLOAD(NGRAPH_CHECK_HELPER, exc_class, ctx, __VA_ARGS__) + +#define GLUE(x, y) x y + +#define RETURN_ARG_COUNT(_1_, \ + _2_, \ + _3_, \ + _4_, \ + _5_, \ + _6, \ + _7, \ + _8, \ + _9, \ + _10, \ + _11, \ + _12, \ + _13, \ + _14, \ + _15, \ + _16, \ + _17, \ + _18, \ + _19, \ + _20, \ + _21, \ + _22, \ + _23, \ + _24, \ + _25, \ + count, \ + ...) \ + count +#define EXPAND_ARGS(args) RETURN_ARG_COUNT args +#define COUNT_ARGS_MAXN(...) \ + EXPAND_ARGS((__VA_ARGS__, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 2, \ + 1, \ + 0)) + +#define OVERLOAD_MACRO2(name, count) name##count +#define OVERLOAD_MACRO1(name, count) OVERLOAD_MACRO2(name, count) +#define OVERLOAD_MACRO(name, count) OVERLOAD_MACRO1(name, count) + +#define CALL_OVERLOAD(name, exc_class, ctx, ...) \ + GLUE(OVERLOAD_MACRO(name, COUNT_ARGS_MAXN(__VA_ARGS__)), (exc_class, ctx, __VA_ARGS__)) diff --git a/src/ngraph/frontend/onnx_import/CMakeLists.txt b/src/ngraph/frontend/onnx_import/CMakeLists.txt index 3d9cb153914..a2b9bedc7ab 100644 --- a/src/ngraph/frontend/onnx_import/CMakeLists.txt +++ b/src/ngraph/frontend/onnx_import/CMakeLists.txt @@ -220,14 +220,15 @@ add_dependencies(onnx_import_interface protobuf::libprotobuf onnx::libonnx onnx: add_dependencies(onnx_import onnx_import_interface) set_property(TARGET onnx_import PROPERTY POSITION_INDEPENDENT_CODE ON) -target_include_directories(onnx_import PRIVATE ${ONNX_IMPORT_INCLUDE_DIR} ${NGRAPH_INCLUDE_PATH} +target_include_directories(onnx_import + SYSTEM PRIVATE ${ONNX_IMPORT_INCLUDE_DIR} ${NGRAPH_INCLUDE_PATH} SYSTEM PRIVATE ${ONNX_INCLUDE_DIR} ${ONNX_PROTO_INCLUDE_DIR} ${Protobuf_INCLUDE_DIR}) target_link_libraries(onnx_import PRIVATE ${Protobuf_LIBRARIES} ${ONNX_PROTO_LIBRARY}) target_compile_definitions(onnx_import PRIVATE ONNX_OPSET_VERSION=${ONNX_OPSET_VERSION}) set_property(TARGET onnx_import_interface PROPERTY POSITION_INDEPENDENT_CODE ON) -target_include_directories(onnx_import_interface PRIVATE ${ONNX_IMPORT_INCLUDE_DIR} ${NGRAPH_INCLUDE_PATH} +target_include_directories(onnx_import_interface SYSTEM PRIVATE ${ONNX_IMPORT_INCLUDE_DIR} ${NGRAPH_INCLUDE_PATH} SYSTEM PRIVATE ${ONNX_INCLUDE_DIR} ${ONNX_PROTO_INCLUDE_DIR} ${Protobuf_INCLUDE_DIR}) target_compile_definitions(onnx_import_interface PRIVATE ONNX_OPSET_VERSION=${ONNX_OPSET_VERSION}) diff --git a/src/ngraph/node.hpp b/src/ngraph/node.hpp index d74b1be4bba..4964e06c5ec 100644 --- a/src/ngraph/node.hpp +++ b/src/ngraph/node.hpp @@ -782,8 +782,8 @@ namespace ngraph bool m_is_short; }; } -#define NODE_VALIDATION_CHECK(node, cond, ...) \ - NGRAPH_CHECK_HELPER(::ngraph::NodeValidationFailure, (node), (cond), ##__VA_ARGS__) +#define NODE_VALIDATION_CHECK(node, ...) \ + NGRAPH_CHECK_HELPER(::ngraph::NodeValidationFailure, (node), __VA_ARGS__) namespace ngraph { diff --git a/test/check.cpp b/test/check.cpp index 62bc618545c..dc273ff36f0 100644 --- a/test/check.cpp +++ b/test/check.cpp @@ -62,7 +62,7 @@ TEST(check, check_with_explanation) catch (const CheckFailure& e) { check_failure_thrown = true; - EXPECT_PRED_FORMAT2(testing::IsSubstring, "Check '(false)' failed at", e.what()); + EXPECT_PRED_FORMAT2(testing::IsSubstring, "Check 'false' failed at", e.what()); EXPECT_PRED_FORMAT2(testing::IsSubstring, "xyzzyxyzzy123", e.what()); } diff --git a/test/type_prop/dyn_replace_slice.cpp b/test/type_prop/dyn_replace_slice.cpp index ba3787b94bf..bc53e1fde27 100644 --- a/test/type_prop/dyn_replace_slice.cpp +++ b/test/type_prop/dyn_replace_slice.cpp @@ -545,7 +545,8 @@ INSTANTIATE_TEST_CASE_P( {1}, {}}, DynReplaceSliceParams{ - {1, 2, 3}, {3}, {3}, {0}, {1, 1, 2}, {0, 0, 1}, {2, 2, 2}, {}, {}, {}, {0}, {2}, {1}})); + {1, 2, 3}, {3}, {3}, {0}, {1, 1, 2}, {0, 0, 1}, {2, 2, 2}, {}, {}, {}, {0}, {2}, {1}}), + PrintToDummyParamName()); void DynReplaceSlice_Test_Shape_Except(const shared_ptr& param_0, const shared_ptr& param_1, diff --git a/test/type_prop/dyn_slice.cpp b/test/type_prop/dyn_slice.cpp index dc404532c27..ce6eb403d9d 100644 --- a/test/type_prop/dyn_slice.cpp +++ b/test/type_prop/dyn_slice.cpp @@ -222,7 +222,8 @@ INSTANTIATE_TEST_CASE_P( {{1}, {1}, {}, {1}, {}}), DynSliceParams({{1, 2, 3}, {3}, {3}, {0}, {1, 1, 2}}, {{0, 0, 1}, {2, 2, 2}, {}}, - {{}, {}, {0}, {2}, {1}}))); + {{}, {}, {0}, {2}, {1}})), + PrintToDummyParamName()); void DynSlice_Test_Shape_Except(const shared_ptr& param_0, const shared_ptr& param_1, diff --git a/test/type_prop/range.cpp b/test/type_prop/range.cpp index 897e3aec15f..e5b785e4608 100644 --- a/test/type_prop/range.cpp +++ b/test/type_prop/range.cpp @@ -436,7 +436,8 @@ INSTANTIATE_TEST_CASE_P(type_prop, RangeParams{1, 23, 2, PartialShape{11}}, RangeParams{1, 22, 2, PartialShape{11}}, RangeParams{0, 0, 1, PartialShape{0}}, - RangeParams{1, 0, 2, PartialShape{0}})); + RangeParams{1, 0, 2, PartialShape{0}}), + PrintToDummyParamName()); struct RangeTestWithNegatives : ::testing::TestWithParam { @@ -488,7 +489,8 @@ INSTANTIATE_TEST_CASE_P(type_prop, RangeParams{2, 0, -1, PartialShape{2}}, RangeParams{-19, 19, 1, PartialShape{38}}, RangeParams{-19, 19, 3, PartialShape{13}}, - RangeParams{20, -19, 1, PartialShape{0}})); + RangeParams{20, -19, 1, PartialShape{0}}), + PrintToDummyParamName()); struct RangeTestFloating : ::testing::TestWithParam { @@ -518,4 +520,5 @@ INSTANTIATE_TEST_CASE_P(type_prop, RangeTestFloating, ::testing::Values(RangeParams{0, 1, 0.25, PartialShape{4}}, RangeParams{-1, 1, 0.25, PartialShape{8}}, - RangeParams{-1, 0.875, 0.25, PartialShape{8}})); + RangeParams{-1, 0.875, 0.25, PartialShape{8}}), + PrintToDummyParamName()); diff --git a/test/util/type_prop.hpp b/test/util/type_prop.hpp index ec579059255..f0e767404b2 100644 --- a/test/util/type_prop.hpp +++ b/test/util/type_prop.hpp @@ -20,3 +20,12 @@ #define EXPECT_HAS_SUBSTRING(haystack, needle) \ EXPECT_PRED_FORMAT2(testing::IsSubstring, needle, haystack) + +struct PrintToDummyParamName +{ + template + std::string operator()(const ::testing::TestParamInfo& info) const + { + return "dummy" + std::to_string(info.index); + } +};