Skip to content

Bump cmake version, fix clang-cl #99

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Bump cmake version, fix clang-cl #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
## Ignore Visual Studio temporary files, build results, and
## files generated by popular Visual Studio add-ons.

#vscode
.vscode

# User-specific files
*.suo
*.user
Expand Down
71 changes: 30 additions & 41 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,69 +1,58 @@
cmake_minimum_required(VERSION 3.0.0 FATAL_ERROR)
cmake_minimum_required(VERSION 3.21.0)
if(POLICY CMP0177)
Copy link
Contributor

@Croydon Croydon Sep 25, 2025

Choose a reason for hiding this comment

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

This is CMake 3.31+ only and could unexpectely change behaviour based on CMake version https://cmake.org/cmake/help/v3.31/policy/CMP0177.html

Seems unnecessary

Copy link
Author

@Febbe Febbe Sep 26, 2025

Choose a reason for hiding this comment

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

The thing is, we already have CMake 4.x and CMake doesn't support this POLICY at some point. So it issues many warnings. It seems to work fine with the new policy if available.

I can test other ways to silence those warnings. Maybe setting that POLICY explicitly to OLD.

cmake_policy(SET CMP0177 NEW)
endif()

project (winflexbison)
project (winflexbison C)

if(NOT MSVC)
message( WARNING "Visual Studio Build supported only" )
message(FATAL_ERROR "Visual Studio Build supported only" )
Copy link
Contributor

@Croydon Croydon Sep 25, 2025

Choose a reason for hiding this comment

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

This might break exotic configurations that actually work, for basically no advantage.

Copy link
Author

@Febbe Febbe Sep 26, 2025

Choose a reason for hiding this comment

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

Ok, will remove it.

endif()

add_definitions(-D_CRT_SECURE_NO_WARNINGS)

if(CMAKE_BUILD_TYPE STREQUAL "Debug")
add_definitions(-D_DEBUG)
endif()

# next line needed for compile in C (nor CPP) mode (ucrt headers bug)
add_definitions(-Dinline=__inline)
# next line needed for VS2017 only
add_definitions(-Drestrict=__restrict)

set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} /W3 /MD /Od /Zi /EHsc")
set(CMAKE_CXX_FLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} /W3 /GL /Od /Oi /Gy /Zi /EHsc")
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} /W3 /EHsc")
set(CMAKE_C_FLAGS_RELEASE "${CMAKE_C_FLAGS_RELEASE} /GL /Oi /Gy")
Comment on lines +18 to +19
Copy link
Author

@Febbe Febbe Sep 26, 2025

Choose a reason for hiding this comment

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

changed this from CXX to C, since all projects are C projects. Those settings had zero effect before.
Shall I remove those lines (behavior unchanged) or use the new flags (lot of warnings, most likely unused intended behavior).


# Define Release by default.
if(NOT CMAKE_BUILD_TYPE)
set(CMAKE_BUILD_TYPE "Release")
message(STATUS "Build type not specified: Use Release by default.")
endif()

if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
if(PROJECT_IS_TOP_LEVEL)
# Output Variables
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_DEBUG "${CMAKE_CURRENT_LIST_DIR}/bin/Debug")
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_RELEASE "${CMAKE_CURRENT_LIST_DIR}/bin/Release")

#------------------------------------------------------------------------
# Static Windows Runtime
# Option to statically link to the Windows runtime. Maybe only
# applies to WIN32/MSVC.
#------------------------------------------------------------------------
if (MSVC)
add_compile_definitions("__extension__")
add_compile_options("/source-charset:utf-8")
option( USE_STATIC_RUNTIME "Set ON to change /MD(DLL) to /MT(static)" OFF )
if (USE_STATIC_RUNTIME)
set(CompilerFlags
CMAKE_CXX_FLAGS
CMAKE_CXX_FLAGS_DEBUG
CMAKE_CXX_FLAGS_RELEASE
CMAKE_C_FLAGS
CMAKE_C_FLAGS_DEBUG
CMAKE_C_FLAGS_RELEASE
)
foreach(CompilerFlag ${CompilerFlags})
string(REPLACE "/MD" "/MT" ${CompilerFlag} "${${CompilerFlag}}")
endforeach()
message(STATUS "Using /MT STATIC runtime")
endif ()
endif ()
endif ()

set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_DEBUG "${CMAKE_CURRENT_BINARY_DIR}/bin/Debug")
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_RELEASE "${CMAKE_CURRENT_BINARY_DIR}/bin/Release")
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY_RELWITHDEBINFO "${CMAKE_CURRENT_BINARY_DIR}/bin/RelWithDebInfo")
endif()
# Only apply to MSVC frontend (not clang frontends)
Copy link
Contributor

@Croydon Croydon Sep 25, 2025

Choose a reason for hiding this comment

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

Spacing is off

if (MSVC AND NOT CMAKE_C_COMPILER_ID STREQUAL "Clang")
# Make __extension__ expand to nothing on MSVC (so GCC/Clang keep the keyword)
add_compile_definitions("__extension__=")
endif()

#if (MSVC)
Copy link
Contributor

@Croydon Croydon Sep 25, 2025

Choose a reason for hiding this comment

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

Those comments can easily be interpreted as actual code and are confusing.

add_compile_options("/source-charset:utf-8")
option(USE_STATIC_RUNTIME "Set ON to change /MD(DLL) to /MT(static)" OFF )
if (USE_STATIC_RUNTIME)
# /MT or /MTd depending on config
set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>")
else()
# /MD or /MDd depending on config
set(CMAKE_MSVC_RUNTIME_LIBRARY "MultiThreaded$<$<CONFIG:Debug>:Debug>DLL")
endif()
#endif ()

add_subdirectory(common)
add_subdirectory(flex)
add_subdirectory(bison)

if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
if(PROJECT_IS_TOP_LEVEL)
# CPACK
if(CMAKE_BUILD_TYPE STREQUAL "Debug")
install(DIRECTORY "${CMAKE_RUNTIME_OUTPUT_DIRECTORY_DEBUG}/" DESTINATION "./")
Expand Down
2 changes: 1 addition & 1 deletion bison/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.0.0 FATAL_ERROR)
cmake_minimum_required(VERSION 3.21.0)

set(PROJECT_NAME win_bison)

Expand Down
1 change: 1 addition & 0 deletions bison/src/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,5 @@ extern char* _stpcpy(char *yydest, const char *yysrc);

extern int strverscmp(const char* s1, const char* s2);

struct obstack;
Copy link
Contributor

@Croydon Croydon Sep 25, 2025

Choose a reason for hiding this comment

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

Why does a CMake version increase require a code change like this?

Copy link
Author

@Febbe Febbe Sep 26, 2025

Choose a reason for hiding this comment

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

Not the cmake bump itself, but that code boils out with clang-cl.

The declaration of struct obstack in the functions parameter list is not visible to the outside. It is basically static.

clang is more strict and interprets it as a different declaration each time this file is included and linked. Therefore we get ODR violations on link time.

extern int obstack_printf(struct obstack* obs, const char* format, ...);
5 changes: 5 additions & 0 deletions buildClangCl.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
mkdir CMakeBuildClangCl
Copy link
Contributor

@Croydon Croydon Sep 25, 2025

Choose a reason for hiding this comment

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

I think we should stop adding more of those bat files. In times where CMake is well integrated in editors, we really shouldn't need them. There are highly unflexible anyway

Copy link
Author

@Febbe Febbe Sep 26, 2025

Choose a reason for hiding this comment

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

Ok, I'll remove it, shall I remove the others too?

Copy link
Contributor

@Croydon Croydon Sep 26, 2025

Choose a reason for hiding this comment

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

Just my make my role here clear: I contributed to winflexbison before, but I have no official say here in any way. So I am stating my own personal opinions here.

I am not sure if someone still relies on the existing bat files, but since the development of winflexbison is mostly dead, it can't affect too many people for sure, if we would remove all of them. But maybe this is something for another pull request.

Copy link
Collaborator

@GitMensch GitMensch Sep 26, 2025

Choose a reason for hiding this comment

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

Another pull request it is... concerning cmake I'll let @Croydon decide, for the rest... stay compatible :-)

cd CMakeBuildClangCl
cmake .. -G"Ninja Multi-Config" -DCMAKE_C_COMPILER=clang-cl.exe
cmake --build . --config "Release" --target package
cd ..
2 changes: 1 addition & 1 deletion common/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.0.0 FATAL_ERROR)
cmake_minimum_required(VERSION 3.20.0)

set(PROJECT_NAME winflexbison_common)

Expand Down
5 changes: 3 additions & 2 deletions common/m4/lib/clean-temp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,9 +136,10 @@ extern int cleanup_temp_subdir (struct temp_dir *dir,
Return 0 upon success, or -1 if there was some problem. */
extern int cleanup_temp_dir_contents (struct temp_dir *dir);

#if !defined (__GNUC__) && !defined (__clang__)
#ifndef _MODE_T_
#define _MODE_T_
typedef int mode_t;
#endif
#endif /* _MODE_T_ */
Copy link
Contributor

@Croydon Croydon Sep 25, 2025

Choose a reason for hiding this comment

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

Seems reasonable, although unrelated.

Copy link
Author

@Febbe Febbe Sep 26, 2025

Choose a reason for hiding this comment

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

It's not unrelated, since it's required to work with clang-cl.
Clang-cl uses vcruntime and MSVC-STL so no mode_t is defined, but clang obviously defines clang.

Copy link
Contributor

@Croydon Croydon Sep 26, 2025

Choose a reason for hiding this comment

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

Sorry, my bad. I was too focused on CMake version adaptations, so I forgot that you want to fix clang-cl midway.


/* ================== Opening and closing temporary files ================== */

Expand Down
2 changes: 1 addition & 1 deletion flex/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
cmake_minimum_required(VERSION 3.0.0 FATAL_ERROR)
cmake_minimum_required(VERSION 3.20.0)
Copy link
Contributor

@Croydon Croydon Sep 25, 2025

Choose a reason for hiding this comment

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

Why do you use 3.20.0 here, if you propose 3.21.0 for the parent project and bison?


set(PROJECT_NAME win_flex)

Expand Down