Skip to content

Conversation

@scc-tw
Copy link

@scc-tw scc-tw commented Mar 5, 2025

Summary:
This pull request improves the handling of MSVC compiler warning flags in the CMake build configuration. Previously, the MSVC compiler warning flags (C_FLAGS and CXX_FLAGS) were unset, resulting in limited warning coverage. This change explicitly defines and applies a comprehensive set of MSVC warnings for improved code quality and consistency.

Key Changes:

  • Removed the placeholder comments and empty flag settings for MSVC (C_FLAGS and CXX_FLAGS).
  • Introduced a dedicated list MSVC_WARNING_FLAGS with a clear set of warning flags:
    • /W4: Comprehensive warnings similar to GCC's -Wall.
    • Enabled critical warnings to detect potential code issues related to declaration hiding (/we4456, /we4457, /we4458).
    • Disabled overly verbose or less useful warnings (/wd4100, /wd4324).

Rationale:

  • This improves the maintainability of the build process by explicitly defining relevant MSVC warnings.
  • It ensures better parity with warning standards established for GCC/Clang builds.

Testing:

  • Successfully compiled and verified locally with MSVC.
  • No negative impact observed on performance benchmarks (llama-perplexity and llama-bench).

Review Considerations:

  • Reviewers should confirm these warnings align with the project's coding standards and practices.

@github-actions github-actions bot added the ggml changes relating to the ggml tensor library for machine learning label Mar 5, 2025
Add MSVC warning flags to improve code quality and catch potential issues
early. The following flags have been added:
- /W4: Comprehensive warnings (similar to -Wall and some of -Wextra)
- /we4456: Declaration hides previous local declaration
- /we4457: Declaration hides function parameter
- /we4458: Declaration hides class member
- /wd4100: Unreferenced formal parameter (too noisy)
- /wd4324: Structure was padded due to alignment specifier
@scc-tw
Copy link
Author

scc-tw commented Mar 5, 2025

Do we need to fix these CI failures? These are expected warnings that Windows MSVC tends to be noisy about, such as:

  • Conversion from 'uint32_t' to 'ggml_fp16_t', possible loss of data
  • Unreachable code
  • Warning C5105: Macro expansion producing 'defined' has undefined behavior
  • etc.

These issues do not occur with GCC or Clang when -Wall -Wextra is enabled.

@scc-tw scc-tw closed this Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ggml changes relating to the ggml tensor library for machine learning

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant