clang/gcc: enable a bunch more warnings (#91)

* clang/gcc: enable -pedantic warnings

* suppress a GCC-specific warning in stb_image

* And some clang-specific warnings

* -Wconversion (clang) -Wold-style-cast (clang+gcc)

and fix a few warnings that show up with these (and a few more with
-Wconversion on gcc, even though that's not enabled by default)

* bunch more warnings

* fixes

* remove merge error

CMakeLists.txt

@@ -97,10 +97,29 @@ if (MSVC)
 else()
     # Activate C++14 only on C++ files, not C files.
     list(APPEND NXT_FLAGS "$<$<STREQUAL:$<TARGET_PROPERTY:LINKER_LANGUAGE>,CXX>:-std=c++14>")
+    # enable -Wold-style-cast on C++
+    list(APPEND NXT_FLAGS "$<$<STREQUAL:$<TARGET_PROPERTY:LINKER_LANGUAGE>,CXX>:-Wold-style-cast>")
     list(APPEND NXT_FLAGS "-fPIC")
 
     list(APPEND NXT_INTERNAL_FLAGS "-Wall" "-Wextra")
+    list(APPEND NXT_INTERNAL_FLAGS "-pedantic")
     list(APPEND NXT_GENERATED_FLAGS "-Wno-unused-variable" "-Wno-unused-function")
+
+    if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang")
+        # don't break the build on older clang versions
+        list(APPEND NXT_INTERNAL_FLAGS "-Wno-error=unknown-warning-option")
+        # GCC's conversion warnings are less useful than clang's
+        list(APPEND NXT_INTERNAL_FLAGS "-Wconversion" "-Wno-sign-conversion")
+        # disable a clang-only -pedantic warning
+        list(APPEND NXT_INTERNAL_FLAGS "-Wno-gnu-zero-variadic-macro-arguments")
+        # additional potentially useful warnings (feel free to remove if they prove un-useful)
+        list(APPEND NXT_INTERNAL_FLAGS "-Wextra-semi")
+        list(APPEND NXT_INTERNAL_FLAGS "-Wstrict-aliasing")
+        list(APPEND NXT_INTERNAL_FLAGS "-Wunreachable-code")
+        list(APPEND NXT_GENERATED_FLAGS "-Wno-unreachable-code")
+        # Probably okay to enable if we establish a field naming convention:
+        #list(APPEND NXT_INTERNAL_FLAGS "-Wshadow")
+    endif()
     if(NXT_USE_WERROR)
         list(APPEND NXT_INTERNAL_FLAGS "-Werror")
     endif()

examples/Animometer.cpp

@@ -130,7 +130,7 @@ void frame() {
     size_t i = 0;
 
     std::vector<nxt::CommandBuffer> commands(50);
-    for (int j = 0; j < 50; j++) {
+    for (size_t j = 0; j < 50; j++) {
 
         nxt::CommandBufferBuilder builder = device.CreateCommandBufferBuilder()
             .BeginRenderPass(renderpass, framebuffer)

examples/CMakeLists.txt

@@ -38,6 +38,9 @@ function(add_nxt_sample target sources)
         set_property(TARGET ${target} APPEND PROPERTY COMPILE_OPTIONS "/wd4456")
         # declaration hides previous declaration -- for picojson
         set_property(TARGET ${target} APPEND PROPERTY COMPILE_OPTIONS "/wd4706")
+    else()
+        # re-enable old style casts -- for GLM
+        set_property(TARGET ${target} APPEND PROPERTY COMPILE_OPTIONS "-Wno-old-style-cast")
     endif()
 endfunction()
 

examples/ComputeBoids.cpp

@@ -81,7 +81,7 @@ void initBuffers() {
         }
     }
 
-    for (int i = 0; i < 2; i++) {
+    for (size_t i = 0; i < 2; i++) {
         particleBuffers[i] = device.CreateBufferBuilder()
             .SetAllowedUsage(nxt::BufferUsageBit::TransferDst | nxt::BufferUsageBit::Vertex | nxt::BufferUsageBit::Storage)
             .SetInitialUsage(nxt::BufferUsageBit::TransferDst)
@@ -261,7 +261,7 @@ void initSim() {
 
 void initCommandBuffers() {
     static const uint32_t zeroOffsets[1] = {0};
-    for (int i = 0; i < 2; ++i) {
+    for (size_t i = 0; i < 2; ++i) {
         auto& bufferSrc = particleBuffers[i];
         auto& bufferDst = particleBuffers[(i + 1) % 2];
         commandBuffers[i] = device.CreateCommandBufferBuilder()

examples/HelloCompute.cpp

@@ -35,7 +35,7 @@ void init() {
     queue = device.CreateQueueBuilder().GetResult();
 
     struct {uint32_t a; float b;} s;
-    memset(&s, sizeof(s), 0);
+    memset(&s, 0, sizeof(s));
     buffer = device.CreateBufferBuilder()
         .SetAllowedUsage(nxt::BufferUsageBit::TransferDst | nxt::BufferUsageBit::Uniform | nxt::BufferUsageBit::Storage)
         .SetInitialUsage(nxt::BufferUsageBit::TransferDst)

examples/HelloTriangle.cpp

@@ -63,7 +63,7 @@ void initTextures() {
     // Initialize the texture with arbitrary data until we can load images
     std::vector<uint8_t> data(4 * 1024 * 1024, 0);
     for (size_t i = 0; i < data.size(); ++i) {
-        data[i] = i % 253;
+        data[i] = static_cast<uint8_t>(i % 253);
     }
 
 

generator/main.py

@@ -225,7 +225,7 @@ class PreprocessingLoader(jinja2.BaseLoader):
             numstarts = (len(self.blockstart.split(line)) - 1) // 2
             indentation_level += numstarts
 
-        return '\n'.join(result)
+        return '\n'.join(result) + '\n'
 
     def remove_indentation(self, line, n):
         for _ in range(n):

generator/templates/apicpp_traits.h

@@ -38,6 +38,6 @@ namespace nxt {
     template<typename BuiltObject>
     using Builder = typename BuiltObjectTrait<BuiltObject>::Builder;
 
-};
+}
 
 #endif // NXTCPP_TRAITS_H

generator/templates/wire/WireClient.cpp

@@ -180,7 +180,7 @@ namespace wire {
                 }
                 void FreeId(uint32_t id) {
                     freeIds.push_back(id);
-                };
+                }
 
                 // 0 is an ID reserved to represent nullptr
                 uint32_t currentId = 1;

src/backend/CommandAllocator.cpp

@@ -186,7 +186,8 @@ namespace backend {
 
             // Make sure we have space for current allocation, plus end of block and alignment padding
             // for the first id.
-            if (!GetNewBlock(nextPtr - currentPtr + sizeof(uint32_t) + alignof(uint32_t))) {
+            ASSERT(nextPtr > currentPtr);
+            if (!GetNewBlock(static_cast<size_t>(nextPtr - currentPtr) + sizeof(uint32_t) + alignof(uint32_t))) {
                 return nullptr;
             }
             return Allocate(commandId, commandSize, commandAlignment);

src/backend/CommandBuffer.cpp

@@ -88,7 +88,7 @@ namespace backend {
         }
 
         uint32_t ComputeDefaultRowPitch(TextureBase* texture, uint32_t width) {
-            uint32_t texelSize = static_cast<uint32_t>(TextureFormatPixelSize(texture->GetFormat()));
+            uint32_t texelSize = TextureFormatPixelSize(texture->GetFormat());
             return texelSize * width;
         }
 
@@ -98,7 +98,7 @@ namespace backend {
                 return false;
             }
 
-            uint32_t texelSize = static_cast<uint32_t>(TextureFormatPixelSize(location.texture.Get()->GetFormat()));
+            uint32_t texelSize = TextureFormatPixelSize(location.texture.Get()->GetFormat());
             if (rowPitch < location.width * texelSize) {
                 builder->HandleError("Row pitch must not be less than the number of bytes per row");
                 return false;

src/backend/CommandBufferStateTracker.cpp

@@ -200,7 +200,7 @@ namespace backend {
 
         aspects.set(VALIDATION_ASPECT_RENDER_SUBPASS);
         return true;
-    };
+    }
 
     bool CommandBufferStateTracker::EndSubpass() {
         if (!aspects[VALIDATION_ASPECT_RENDER_SUBPASS]) {
@@ -233,7 +233,7 @@ namespace backend {
         aspects.reset(VALIDATION_ASPECT_RENDER_SUBPASS);
         UnsetPipeline();
         return true;
-    };
+    }
 
     bool CommandBufferStateTracker::BeginRenderPass(RenderPassBase* renderPass, FramebufferBase* framebuffer) {
         if (aspects[VALIDATION_ASPECT_COMPUTE_PASS]) {
@@ -402,7 +402,7 @@ namespace backend {
         }
         auto it = mostRecentBufferUsages.find(buffer);
         return it != mostRecentBufferUsages.end() && (it->second & usage);
-    };
+    }
 
     bool CommandBufferStateTracker::TextureHasGuaranteedUsageBit(TextureBase* texture, nxt::TextureUsageBit usage) const {
         ASSERT(usage != nxt::TextureUsageBit::None && nxt::HasZeroOrOneBits(usage));
@@ -411,7 +411,7 @@ namespace backend {
         }
         auto it = mostRecentTextureUsages.find(texture);
         return it != mostRecentTextureUsages.end() && (it->second & usage);
-    };
+    }
 
     bool CommandBufferStateTracker::IsInternalTextureTransitionPossible(TextureBase* texture, nxt::TextureUsageBit usage) const {
         ASSERT(usage != nxt::TextureUsageBit::None && nxt::HasZeroOrOneBits(usage));
@@ -419,7 +419,7 @@ namespace backend {
             return false;
         }
         return texture->IsTransitionPossible(usage);
-    };
+    }
 
     bool CommandBufferStateTracker::IsExplicitTextureTransitionPossible(TextureBase* texture, nxt::TextureUsageBit usage) const {
         const nxt::TextureUsageBit attachmentUsages =
@@ -519,7 +519,7 @@ namespace backend {
             }
         }
         return true;
-    };
+    }
 
     bool CommandBufferStateTracker::RevalidateCanDraw() {
         if (!aspects[VALIDATION_ASPECT_RENDER_PIPELINE]) {

src/backend/Pipeline.cpp

@@ -36,7 +36,7 @@ namespace backend {
             info->mask = moduleInfo.mask;
 
             for (uint32_t i = 0; i < moduleInfo.names.size(); i++) {
-                unsigned int size = moduleInfo.sizes[i];
+                uint32_t size = moduleInfo.sizes[i];
                 if (size == 0) {
                     continue;
                 }

src/backend/ShaderModule.cpp

@@ -96,7 +96,7 @@ namespace backend {
 
         // Fill in bindingInfo with the SPIRV bindings
         auto ExtractResourcesBinding = [this](const std::vector<spirv_cross::Resource>& resources,
-                                              const spirv_cross::Compiler& compiler, nxt::BindingType type) {
+                                              const spirv_cross::Compiler& compiler, nxt::BindingType bindingType) {
             constexpr uint64_t requiredBindingDecorationMask = (1ull << spv::DecorationBinding) | (1ull << spv::DecorationDescriptorSet);
 
             for (const auto& resource : resources) {
@@ -113,7 +113,7 @@ namespace backend {
                 info.used = true;
                 info.id = resource.id;
                 info.base_type_id = resource.base_type_id;
-                info.type = type;
+                info.type = bindingType;
             }
         };
 

src/backend/ShaderModule.h

@@ -42,7 +42,7 @@ namespace backend {
                 std::bitset<kMaxPushConstants> mask;
 
                 std::array<std::string, kMaxPushConstants> names;
-                std::array<int, kMaxPushConstants> sizes;
+                std::array<uint32_t, kMaxPushConstants> sizes;
                 std::array<PushConstantType, kMaxPushConstants> types;
             };
 

src/backend/Texture.cpp

@@ -19,7 +19,7 @@
 
 namespace backend {
 
-    size_t TextureFormatPixelSize(nxt::TextureFormat format) {
+    uint32_t TextureFormatPixelSize(nxt::TextureFormat format) {
         switch (format) {
             case nxt::TextureFormat::R8G8B8A8Unorm:
                 return 4;

src/backend/Texture.h

@@ -23,7 +23,7 @@
 
 namespace backend {
 
-    size_t TextureFormatPixelSize(nxt::TextureFormat format);
+    uint32_t TextureFormatPixelSize(nxt::TextureFormat format);
     bool TextureFormatHasDepth(nxt::TextureFormat format);
     bool TextureFormatHasStencil(nxt::TextureFormat format);
 

src/backend/metal/InputStateMTL.mm

@@ -48,7 +48,7 @@ namespace metal {
         mtlVertexDescriptor = [MTLVertexDescriptor new];
 
         const auto& attributesSetMask = GetAttributesSetMask();
-        for (size_t i = 0; i < attributesSetMask.size(); ++i) {
+        for (uint32_t i = 0; i < attributesSetMask.size(); ++i) {
             if (!attributesSetMask[i]) {
                 continue;
             }
@@ -63,7 +63,7 @@ namespace metal {
         }
 
         const auto& inputsSetMask = GetInputsSetMask();
-        for (size_t i = 0; i < inputsSetMask.size(); ++i) {
+        for (uint32_t i = 0; i < inputsSetMask.size(); ++i) {
             if (!inputsSetMask[i]) {
                 continue;
             }

src/backend/opengl/CommandBufferGL.cpp

@@ -181,7 +181,7 @@ namespace opengl {
                         glBindTexture(target, texture->GetHandle());
 
                         ASSERT(texture->GetDimension() == nxt::TextureDimension::e2D);
-                        glPixelStorei(GL_UNPACK_ROW_LENGTH, copy->rowPitch / static_cast<uint32_t>(TextureFormatPixelSize(texture->GetFormat())));
+                        glPixelStorei(GL_UNPACK_ROW_LENGTH, copy->rowPitch / TextureFormatPixelSize(texture->GetFormat()));
                         glTexSubImage2D(target, dst.level, dst.x, dst.y, dst.width, dst.height,
                                         format.format, format.type,
                                         reinterpret_cast<void*>(static_cast<uintptr_t>(src.offset)));
@@ -212,7 +212,7 @@ namespace opengl {
                                                texture->GetHandle(), src.level);
 
                         glBindBuffer(GL_PIXEL_PACK_BUFFER, buffer->GetHandle());
-                        glPixelStorei(GL_PACK_ROW_LENGTH, copy->rowPitch / static_cast<uint32_t>(TextureFormatPixelSize(texture->GetFormat())));
+                        glPixelStorei(GL_PACK_ROW_LENGTH, copy->rowPitch / TextureFormatPixelSize(texture->GetFormat()));
                         ASSERT(src.depth == 1 && src.z == 0);
                         void* offset = reinterpret_cast<void*>(static_cast<uintptr_t>(dst.offset));
                         glReadPixels(src.x, src.y, src.width, src.height, format.format, format.type, offset);

src/backend/opengl/PipelineGL.cpp

@@ -192,12 +192,12 @@ namespace opengl {
     }
 
     const std::vector<GLuint>& PipelineGL::GetTextureUnitsForSampler(GLuint index) const {
-        ASSERT(index >= 0 && index < unitsForSamplers.size());
+        ASSERT(index < unitsForSamplers.size());
         return unitsForSamplers[index];
     }
 
     const std::vector<GLuint>& PipelineGL::GetTextureUnitsForTexture(GLuint index) const {
-        ASSERT(index >= 0 && index < unitsForSamplers.size());
+        ASSERT(index < unitsForSamplers.size());
         return unitsForTextures[index];
     }
 

src/common/BitSetIterator.h

@@ -112,7 +112,7 @@ unsigned long BitSetIterator<N, T>::Iterator::getNextBit() {
     static std::bitset<N> wordMask(std::numeric_limits<uint32_t>::max());
 
     while (mOffset < N) {
-        uint32_t wordBits = (mBits & wordMask).to_ulong();
+        uint32_t wordBits = static_cast<uint32_t>((mBits & wordMask).to_ulong());
         if (wordBits != 0ul) {
             return ScanForward(wordBits) + mOffset;
         }

src/common/Math.cpp

@@ -28,7 +28,7 @@ uint32_t ScanForward(uint32_t bits) {
         ASSERT(ret != 0);
         return firstBitIndex;
     #else
-        return static_cast<unsigned long>(__builtin_ctz(bits));
+        return static_cast<uint32_t>(__builtin_ctz(bits));
     #endif
 }
 
@@ -40,7 +40,7 @@ uint32_t Log2(uint32_t value) {
         ASSERT(ret != 0);
         return firstBitIndex;
     #else
-        return 31 - __builtin_clz(value);
+        return 31 - static_cast<uint32_t>(__builtin_clz(value));
     #endif
 }
 
@@ -52,13 +52,13 @@ bool IsPowerOfTwo(size_t n) {
 bool IsAligned(const void* ptr, size_t alignment) {
     ASSERT(IsPowerOfTwo(alignment));
     ASSERT(alignment != 0);
-    return (reinterpret_cast<intptr_t>(ptr) & (alignment - 1)) == 0;
+    return (reinterpret_cast<size_t>(ptr) & (alignment - 1)) == 0;
 }
 
 void* AlignVoidPtr(void* ptr, size_t alignment) {
     ASSERT(IsPowerOfTwo(alignment));
     ASSERT(alignment != 0);
-    return reinterpret_cast<void*>((reinterpret_cast<intptr_t>(ptr) + (alignment - 1)) & ~(alignment - 1));
+    return reinterpret_cast<void*>((reinterpret_cast<size_t>(ptr) + (alignment - 1)) & ~(alignment - 1));
 }
 
 uint32_t Align(uint32_t value, size_t alignment) {

src/tests/unittests/CommandAllocatorTests.cpp

@@ -203,7 +203,7 @@ TEST(CommandAllocator, LargeCommands) {
 
     const int kCommandCount = 5;
 
-    int count = 0;
+    uint32_t count = 0;
     for (int i = 0; i < kCommandCount; i++) {
         CommandBig* big = allocator.Allocate<CommandBig>(CommandType::Big);
         for (int j = 0; j < kBigBufferSize; j++) {
@@ -260,14 +260,15 @@ TEST(CommandAllocator, ManySmallCommands) {
     iterator.DataWasDestroyed();
 }
 
-//        ________
+/*        ________
-//       /        \
+ *       /        \
-//       | POUIC! |
+ *       | POUIC! |
-//       \_ ______/
+ *       \_ ______/
-//         v
+ *         v
-//    ()_()
+ *    ()_()
-//    (O.o)
+ *    (O.o)
-//    (> <)o
+ *    (> <)o
+ */
 
 // Test usage of iterator.Reset
 TEST(CommandAllocator, IteratorReset) {

src/tests/unittests/MathTests.cpp

@@ -64,7 +64,7 @@ TEST(Math, AlignPtr) {
 
         ASSERT_GE(aligned - unaligned, 0);
         ASSERT_LT(static_cast<size_t>(aligned - unaligned), kTestAlignment);
-        ASSERT_EQ(reinterpret_cast<intptr_t>(aligned) & (kTestAlignment -1), 0);
+        ASSERT_EQ(reinterpret_cast<uintptr_t>(aligned) & (kTestAlignment -1), 0);
     }
 }
 

src/utils/CMakeLists.txt

@@ -51,3 +51,7 @@ add_library(utils STATIC ${UTILS_SOURCES})
 target_link_libraries(utils nxt_backend shaderc nxtcpp nxt)
 target_include_directories(utils PUBLIC ${SRC_DIR})
 NXTInternalTarget("" utils)
+if(NOT MSVC)
+    # allow C-style casts -- for shaderc
+    set_property(TARGET utils APPEND PROPERTY COMPILE_OPTIONS "-Wno-old-style-cast")
+endif()

src/utils/NXTHelpers.cpp

@@ -50,8 +50,13 @@ namespace utils {
             return;
         }
 
-        size_t size = (result.cend() - result.cbegin());
+        // result.cend and result.cbegin return pointers to uint32_t.
-        builder.SetSource(static_cast<uint32_t>(size), result.cbegin());
+        const uint32_t* resultBegin = result.cbegin();
+        const uint32_t* resultEnd = result.cend();
+        // So this size is in units of sizeof(uint32_t).
+        ptrdiff_t resultSize = resultEnd - resultBegin;
+        // SetSource takes data as uint32_t*.
+        builder.SetSource(static_cast<uint32_t>(resultSize), result.cbegin());
 
 #ifdef DUMP_SPIRV_ASSEMBLY
         {

src/utils/SystemUtils.cpp

@@ -25,11 +25,11 @@
 namespace utils {
 
     #if defined(NXT_PLATFORM_WINDOWS)
-        void USleep(int usecs) {
+        void USleep(unsigned int usecs) {
             Sleep(static_cast<DWORD>(usecs / 1000));
         }
     #elif defined(NXT_PLATFORM_POSIX)
-        void USleep(int usecs) {
+        void USleep(unsigned int usecs) {
             usleep(usecs);
         }
     #else

src/utils/SystemUtils.h

@@ -14,6 +14,6 @@
 
 namespace utils {
 
-    void USleep(int usecs);
+    void USleep(unsigned int usecs);
 
 }

third_party/CMakeLists.txt

@@ -25,7 +25,7 @@ set(GTEST_DIR ${CMAKE_CURRENT_SOURCE_DIR}/googletest/googletest)
 set(GMOCK_DIR ${CMAKE_CURRENT_SOURCE_DIR}/googletest/googlemock)
 add_library(gtest STATIC ${GTEST_DIR}/src/gtest-all.cc ${GMOCK_DIR}/src/gmock-all.cc)
 target_include_directories(gtest SYSTEM PUBLIC ${GTEST_DIR}/include ${GMOCK_DIR}/include)
-target_include_directories(gtest PRIVATE ${GTEST_DIR} ${GMOCK_DIR})
+target_include_directories(gtest SYSTEM PRIVATE ${GTEST_DIR} ${GMOCK_DIR})
 find_package(Threads)
 target_link_libraries(gtest ${CMAKE_THREAD_LIBS_INIT})
 NXTExternalTarget("third_party" gtest)
@@ -36,7 +36,7 @@ add_library(glad STATIC
     ${CMAKE_CURRENT_SOURCE_DIR}/glad/include/glad/glad.h
     ${CMAKE_CURRENT_SOURCE_DIR}/glad/include/KHR/khrplatform.h
 )
-target_include_directories(glad PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/glad/include)
+target_include_directories(glad SYSTEM PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/glad/include)
 NXTExternalTarget("third_party" glad)
 
 # ShaderC
@@ -90,7 +90,7 @@ add_library(spirv_cross STATIC
     ${CMAKE_CURRENT_SOURCE_DIR}/spirv-cross/spirv_hlsl.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/spirv-cross/spirv_hlsl.hpp
 )
-target_include_directories(spirv_cross PUBLIC ${CMAKE_CURRENT_SOURCE_DIR})
+target_include_directories(spirv_cross SYSTEM PUBLIC ${CMAKE_CURRENT_SOURCE_DIR})
 NXTExternalTarget("third_party" spirv_cross)
 
 # STB, used for stb_image