Skip to content

Conversation

IAmNotHanni
Copy link
Contributor

@IAmNotHanni IAmNotHanni commented Apr 9, 2025

  • Use clang-tidy to perform static code analysis
  • Ignore warnings -modernize-use-trailing-return-type, cppcoreguidelines-macro-usage, modernize-use-auto

clang-tidy is very powerful, but it needs proper configuration because you sometimes disagree with some things it comes up with. Let me know if you have further warnings to add to the ignore list. Still, I find it interesting what it already can find.

@IAmNotHanni
Copy link
Contributor Author

@adam-sawicki-a adam-sawicki-a merged commit bcd776c into GPUOpen-LibrariesAndSDKs:master Apr 10, 2025
7 checks passed
adam-sawicki-a added a commit that referenced this pull request Apr 10, 2025
@adam-sawicki-a adam-sawicki-a added next release To be done as soon as possible quality Code quality improvement e.g. refactoring labels Apr 10, 2025
@adam-sawicki-a
Copy link
Contributor

Thank you for this contribution.

I made some fixes in the code based on the warnings found. For the rest of the warnings, I think we can just leave them as-is and ignore them.

Warnings that I consider the worst and worth disabling are:

/home/runner/work/VulkanMemoryAllocator/VulkanMemoryAllocator/include/vk_mem_alloc.h:2821:1: warning: system include cstdint not allowed, transitively included from /home/runner/work/VulkanMemoryAllocator/VulkanMemoryAllocator/include/vk_mem_alloc.h [llvmlibc-restrict-system-libc-headers]
#include
^~~~~~~~~~~~~~~~~~

Why shouldn't we use standard C++ headers? Is it something LLVM-specific? We are not compiling LLVM here.

Likely the same with llvmlibc-callee-namespace.

modernize-use-nodiscard: VMA declares compatibility with C++14, so we shouldn't have a warning about the usage of [[nodiscard]] that has been added in C++17.

warning: kernel performance could be improved by unrolling this loop with a '#pragma unroll' directive [altera-unroll-loops]

What is this about? Something specific to Altera platform? Adding #pragma is not a good idea in a code that should be multiplatform.

warning: do not access members of unions; use (boost::)variant instead [cppcoreguidelines-pro-type-union-access]

Adding dependency on Boost is not a good idea, while std::variant is added only in C++17.

Besides this, why do we have warnings about file /home/runner/work/VulkanMemoryAllocator/VulkanMemoryAllocator/build/CMakeFiles/3.31.6/CompilerIdCXX/CMakeCXXCompilerId.cpp? Why it also gets analyzed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

next release To be done as soon as possible quality Code quality improvement e.g. refactoring

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants