Repository: NVIDIA-Genomics-Research/GenomeWorks (language: python)

Issue:
[cga] Centralize compilation flags
Currently every module defines its own compilation flags. In some cases this is the wanted behavior, but in others (C++ standard, warning level..) it causes confusion.

Think about which flags should be set for the whole SDK.


Discussion / hints:
Not a cmake expert, but I think the cleanest way would be to define a central interface library `add_library(cga_common INTERFACE)`, set the properties there via `target_compile_options(cga_common INTERFACE <flags>)` & friends, and then link all our actual targets against this interface library. There was a nice talk by Robert Maynard on Modern CMake + CUDA at last year's GTC https://developer.nvidia.com/gtc/2019/video/S9444.

Fix patch:
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 414c78dfd..275c8837c 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -68,10 +68,10 @@ else()
     set_property(GLOBAL PROPERTY gw_library_type STATIC)
 endif()
 
-include(cmake/CXX.cmake)
 include(cmake/CUDA.cmake)
-include(cmake/Doxygen.cmake)
 include(cmake/3rdparty.cmake)
+include(cmake/CXX.cmake) # Must occur after 3rdparty modules are added
+include(cmake/Doxygen.cmake)
 include(cmake/Tests.cmake)
 include(cmake/Benchmarks.cmake)
 include(cmake/Format.cmake)
diff --git a/cmake/CXX.cmake b/cmake/CXX.cmake
index fc8a06a1b..04bbfd328 100644
--- a/cmake/CXX.cmake
+++ b/cmake/CXX.cmake
@@ -21,3 +21,6 @@ set(CMAKE_CXX_STANDARD 17)
 
 #Add OpenMP
 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fopenmp")
+
+# Add common copmiler checks
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror -Wall -Wextra")
diff --git a/cmake/Format.cmake b/cmake/Format.cmake
index f066095bf..e349fc4c5 100644
--- a/cmake/Format.cmake
+++ b/cmake/Format.cmake
@@ -86,7 +86,7 @@ function(gw_enable_formatting_targets)
         endforeach()
 
         add_custom_target(format DEPENDS ${format_list} COMMENT "Format source files")
-        add_custom_target(check-format DEPENDS ${format_check_list} COMMENT "Check format of source files")
+        add_custom_target(check-format ALL DEPENDS ${format_check_list} COMMENT "Check format of source files")
     else()
         message(STATUS "clang-format not found. Auto-formatting disabled.")
     endif()
diff --git a/common/base/include/claraparabricks/genomeworks/utils/limits.cuh b/common/base/include/claraparabricks/genomeworks/utils/limits.cuh
index e366ae6ee..7676b0d23 100644
--- a/common/base/include/claraparabricks/genomeworks/utils/limits.cuh
+++ b/common/base/include/claraparabricks/genomeworks/utils/limits.cuh
@@ -26,6 +26,7 @@ namespace claraparabricks
 namespace genomeworks
 {
 #ifdef GW_CUDA_BEFORE_10_1
+#pragma GCC diagnostic ignored "-Wignored-qualifiers"
 template <typename T>
 struct numeric_limits
 {
diff --git a/common/io/CMakeLists.txt b/common/io/CMakeLists.txt
index 8fbe178db..0dc14f6e9 100644
--- a/common/io/CMakeLists.txt
+++ b/common/io/CMakeLists.txt
@@ -34,8 +34,6 @@ target_include_directories(${MODULE_NAME}
         $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
 )
 
-target_compile_options(${MODULE_NAME} PRIVATE -Werror)
-
 install(TARGETS ${MODULE_NAME} 
     EXPORT ${MODULE_NAME}
     DESTINATION lib
diff --git a/cudaaligner/CMakeLists.txt b/cudaaligner/CMakeLists.txt
index 781394386..dfaffad08 100644
--- a/cudaaligner/CMakeLists.txt
+++ b/cudaaligner/CMakeLists.txt
@@ -51,7 +51,6 @@ target_include_directories(${MODULE_NAME}_internal INTERFACE
 
 target_link_libraries(${MODULE_NAME} gwbase cub libcudacxx)
 
-target_compile_options(${MODULE_NAME} PRIVATE -Wall -Wextra)
 if (gw_optimize_for_native_cpu)
     target_compile_options(${MODULE_NAME} PRIVATE -march=native)
 endif()
diff --git a/cudaaligner/benchmarks/main.cpp b/cudaaligner/benchmarks/main.cpp
index 8f39df298..15b15518f 100644
--- a/cudaaligner/benchmarks/main.cpp
+++ b/cudaaligner/benchmarks/main.cpp
@@ -80,6 +80,8 @@ typename std::enable_if<!std::is_same<AlignerT, AlignerGlobalMyersBanded>::value
 template <typename AlignerT>
 typename std::enable_if<std::is_same<AlignerT, AlignerGlobalMyersBanded>::value, std::unique_ptr<Aligner>>::type create_aligner_tmp_dispatch(int32_t genome_size, int32_t alignments_per_batch, DefaultDeviceAllocator allocator, cudaStream_t stream, int32_t device_id)
 {
+    static_cast<void>(genome_size);
+    static_cast<void>(alignments_per_batch);
     const int64_t max_device_memory = -1;
     const int32_t max_bandwidth     = 1024;
     return std::make_unique<AlignerT>(
diff --git a/cudaaligner/samples/sample_cudaaligner.cpp b/cudaaligner/samples/sample_cudaaligner.cpp
index fb147876e..f46ce4447 100644
--- a/cudaaligner/samples/sample_cudaaligner.cpp
+++ b/cudaaligner/samples/sample_cudaaligner.cpp
@@ -57,7 +57,7 @@ std::unique_ptr<Aligner> initialize_batch(int32_t max_query_size,
                                                     stream,
                                                     device_id);
 
-    return std::move(batch);
+    return batch;
 }
 
 void generate_data(std::vector<std::pair<std::string, std::string>>& data,
diff --git a/cudaaligner/src/myers_gpu.cu b/cudaaligner/src/myers_gpu.cu
index edf4dc659..4f80ea077 100644
--- a/cudaaligner/src/myers_gpu.cu
+++ b/cudaaligner/src/myers_gpu.cu
@@ -14,6 +14,9 @@
 * limitations under the License.
 */
 
+// This is required for the unused parameter error raised by cuda/atomic
+#pragma GCC diagnostic ignored "-Wunused-parameter"
+
 #include "myers_gpu.cuh"
 #include "batched_device_matrices.cuh"
 
diff --git a/cudaextender/CMakeLists.txt b/cudaextender/CMakeLists.txt
index 8c00a1147..c0ba79a7c 100644
--- a/cudaextender/CMakeLists.txt
+++ b/cudaextender/CMakeLists.txt
@@ -47,7 +47,6 @@ cuda_add_library(${MODULE_NAME} ${gw_library_type}
 
 target_link_libraries(${MODULE_NAME} gwbase gwio cub)
 
-target_compile_options(${MODULE_NAME} PRIVATE -Werror -Wall -Wextra)
 if (gw_optimize_for_native_cpu)
     target_compile_options(${MODULE_NAME} PRIVATE -march=native)
 endif ()
diff --git a/cudamapper/CMakeLists.txt b/cudamapper/CMakeLists.txt
index 1ec17f891..ae0044088 100644
--- a/cudamapper/CMakeLists.txt
+++ b/cudamapper/CMakeLists.txt
@@ -75,7 +75,6 @@ else()
      target_link_libraries(${MODULE_NAME} gwbase gwio cub)
 endif()
 
-target_compile_options(${MODULE_NAME} PRIVATE -Werror -Wall -Wextra)
 if (gw_optimize_for_native_cpu)
     target_compile_options(${MODULE_NAME} PRIVATE -march=native)
 endif()
diff --git a/cudamapper/samples/sample_cudamapper.cpp b/cudamapper/samples/sample_cudamapper.cpp
index 142439d4b..7839e16b8 100644
--- a/cudamapper/samples/sample_cudamapper.cpp
+++ b/cudamapper/samples/sample_cudamapper.cpp
@@ -57,11 +57,8 @@ namespace cudamapper
 
 namespace
 {
-std::vector<IndexDescriptor> initialize_batch(const std::shared_ptr<const io::FastaParser> parser,
-                                              DefaultDeviceAllocator allocator)
+std::vector<IndexDescriptor> initialize_batch(const std::shared_ptr<const io::FastaParser> parser)
 {
-    //Init();
-
     // group reads into indices. For small inputs, there will only be 1 index
     std::vector<IndexDescriptor> index_descriptors = group_reads_into_indices(*parser,
                                                                               INDEX_SIZE * 1'000'000);
@@ -211,8 +208,8 @@ int main(int argc, char** argv)
     target_parser = query_parser;                                                           // assume all to all
 
     // group the indices
-    std::vector<IndexDescriptor> query_index_descriptors  = initialize_batch(query_parser, allocator);
-    std::vector<IndexDescriptor> target_index_descriptors = initialize_batch(target_parser, allocator);
+    std::vector<IndexDescriptor> query_index_descriptors  = initialize_batch(query_parser);
+    std::vector<IndexDescriptor> target_index_descriptors = initialize_batch(target_parser);
 
     process_batch(query_index_descriptors, target_index_descriptors, query_parser, target_parser, allocator, print, format);
 
diff --git a/cudapoa/CMakeLists.txt b/cudapoa/CMakeLists.txt
index 529a765cc..e538f76a2 100644
--- a/cudapoa/CMakeLists.txt
+++ b/cudapoa/CMakeLists.txt
@@ -44,7 +44,6 @@ cuda_add_library(${MODULE_NAME} ${gw_library_type}
 
 target_link_libraries(${MODULE_NAME} gwbase gwio)
 
-target_compile_options(${MODULE_NAME} PRIVATE -Werror -Wall -Wextra)
 if (gw_optimize_for_native_cpu)
     target_compile_options(${MODULE_NAME} PRIVATE -march=native)
 endif()
diff --git a/cudapoa/benchmarks/main.cpp b/cudapoa/benchmarks/main.cpp
index faddd437f..fdd609841 100644
--- a/cudapoa/benchmarks/main.cpp
+++ b/cudapoa/benchmarks/main.cpp
@@ -47,8 +47,6 @@ static void BM_SingleBatchTest(benchmark::State& state)
 
 static void CustomArguments(benchmark::internal::Benchmark* b)
 {
-    const int32_t min_total_windows = 512;
-    const int32_t max_total_windows = 4096;
     for (int32_t batches = 1; batches <= 16; batches *= 2)
     {
         b->Args({batches});
diff --git a/cudapoa/include/claraparabricks/genomeworks/cudapoa/utils.hpp b/cudapoa/include/claraparabricks/genomeworks/cudapoa/utils.hpp
index ac23d22a2..08ca2aeb4 100644
--- a/cudapoa/include/claraparabricks/genomeworks/cudapoa/utils.hpp
+++ b/cudapoa/include/claraparabricks/genomeworks/cudapoa/utils.hpp
@@ -91,7 +91,7 @@ inline void resize_windows(std::vector<std::vector<std::string>>& windows, const
             }
         }
 
-        assert(windows.size() == total_windows);
+        assert(get_size(windows) == total_windows);
     }
 }
 
diff --git a/cudapoa/samples/sample_cudapoa.cpp b/cudapoa/samples/sample_cudapoa.cpp
index 773e8ee17..863d4b832 100644
--- a/cudapoa/samples/sample_cudapoa.cpp
+++ b/cudapoa/samples/sample_cudapoa.cpp
@@ -58,7 +58,7 @@ std::unique_ptr<Batch> initialize_batch(bool msa, const BatchConfig& batch_size)
                                                 mismatch_score,
                                                 match_score);
 
-    return std::move(batch);
+    return batch;
 }
 
 void process_batch(Batch* batch, bool msa_flag, bool print, std::vector<int32_t>& list_of_group_ids, int id_offset)
diff --git a/cudapoa/src/application_parameters.cpp b/cudapoa/src/application_parameters.cpp
index 797c6f71d..d6531fb7b 100644
--- a/cudapoa/src/application_parameters.cpp
+++ b/cudapoa/src/application_parameters.cpp
@@ -113,8 +113,10 @@ ApplicationParameters::ApplicationParameters(int argc, char* argv[])
             break;
         case 'v':
             print_version();
+            break;
         case 'h':
             help(0);
+            break;
         default:
             exit(1);
         }
diff --git a/cudapoa/src/main.cpp b/cudapoa/src/main.cpp
index 181ea5f1c..5ae96a49e 100644
--- a/cudapoa/src/main.cpp
+++ b/cudapoa/src/main.cpp
@@ -61,7 +61,7 @@ std::unique_ptr<Batch> initialize_batch(int32_t mismatch_score,
                                                 mismatch_score,
                                                 match_score);
 
-    return std::move(batch);
+    return batch;
 }
 
 void process_batch(Batch* batch, bool msa_flag, bool print, std::vector<int32_t>& list_of_group_ids, int id_offset)


Test patch:
diff --git a/cudaaligner/tests/Test_ApproximateBandedMyers.cpp b/cudaaligner/tests/Test_ApproximateBandedMyers.cpp
index 2492d24b9..f1289208e 100644
--- a/cudaaligner/tests/Test_ApproximateBandedMyers.cpp
+++ b/cudaaligner/tests/Test_ApproximateBandedMyers.cpp
@@ -26,17 +26,6 @@ namespace
 {
 
 constexpr int32_t word_size = 32;
-int32_t get_max_sequence_length(std::vector<std::pair<std::string, std::string>> const& inputs)
-{
-    using claraparabricks::genomeworks::get_size;
-    int64_t max_string_size = 0;
-    for (auto const& pair : inputs)
-    {
-        max_string_size = std::max(max_string_size, get_size(pair.first));
-        max_string_size = std::max(max_string_size, get_size(pair.second));
-    }
-    return static_cast<int32_t>(max_string_size);
-}
 
 struct TestCase
 {
@@ -139,8 +128,6 @@ TEST_P(TestApproximateBandedMyers, EditDistanceMonotonicallyDecreasesWithBandWid
 
     DefaultDeviceAllocator allocator = create_default_device_allocator();
 
-    const int32_t max_string_size = std::max(get_size<int32_t>(t.query), get_size<int32_t>(t.target));
-
     int32_t last_edit_distance      = std::numeric_limits<int32_t>::max();
     int32_t last_bw                 = -1;
     std::vector<int32_t> bandwidths = {2, 4, 16, 31, 32, 34, 63, 64, 66, 255, 256, 258, 1023, 1024, 1026, 2048};
diff --git a/cudamapper/tests/CMakeLists.txt b/cudamapper/tests/CMakeLists.txt
index da8617978..3e69d6fde 100644
--- a/cudamapper/tests/CMakeLists.txt
+++ b/cudamapper/tests/CMakeLists.txt
@@ -31,6 +31,9 @@ set(SOURCES
     Test_CudamapperUtilsKmerFunctions.cpp
    )
 
+# This is a workaround for an "unnecessary parantheses" error raised when compiling the gmock.h header.
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-parentheses")
+
 get_property(cudamapper_data_include_dir GLOBAL PROPERTY cudamapper_data_include_dir)
 include_directories(${cudamapper_data_include_dir})
 
diff --git a/cudapoa/tests/Test_CudapoaAddAlignment.cu b/cudapoa/tests/Test_CudapoaAddAlignment.cu
index 6a807cd62..ff221ed48 100644
--- a/cudapoa/tests/Test_CudapoaAddAlignment.cu
+++ b/cudapoa/tests/Test_CudapoaAddAlignment.cu
@@ -41,6 +41,7 @@ public:
                    std::vector<uint8_t> read, std::vector<int8_t> base_weights, std::vector<int16_t> alignment_graph, std::vector<int16_t> alignment_read)
         : graph(nodes, outgoing_edges, node_alignments, node_coverage_counts)
         , read_(read)
+        , base_weights_(base_weights)
         , alignment_graph_(alignment_graph)
         , alignment_read_(alignment_read)
     {