Skip to content

Conversation

@npajkovsky
Copy link

No description provided.

Signed-off-by: Nikola Pajkovsky <nikolap@openssl.org>
@npajkovsky npajkovsky requested review from Sashan and esyr November 7, 2025 11:54
Copy link
Contributor

@Sashan Sashan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense. thanks.

Copy link
Member

@esyr esyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to see changes in the source files as a separate preparatory commit, but I guess it's fine here. Otherwise, the only meaningful change needed is the directive ordering, everything else is optional.

# [1] https://cmake.org/cmake/help/v4.0/release/4.0.html#other-changes
#

set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move it after cmake_minimum_required and project directives and add CACHE and FORCE parameters (so it can be overridden from the command line).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'm not sure that cl supports -Wall, it should be /Wall or something like that, rather, so, probably, it would better be something like that:

if(WIN32)
    set(CFLAGS_WARNINGS "/Wall" CACHE STRING "Compiler options for warnings")
else()
    set(CFLAGS_WARNINGS "-Wall" CACHE STRING "Compiler options for warnings")
endif()

set(set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${CFLAGS_WARNINGS}"
    CACHE STRING "C compiler options" FORCE)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants