Skip to content

Commit

Permalink
Set FMT_CAN_MODULE=OFF for MSVC 19.29.30035+
Browse files Browse the repository at this point in the history
  • Loading branch information
@spyridon97 @vitaut
spyridon97 authored and vitaut committed Jul 23, 2021
1 parent 63fe2d5 commit 3def950
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ set(FMT_CAN_MODULE OFF)
if (CMAKE_CXX_STANDARD GREATER 17 AND
# msvc 16.10-pre4
MSVC AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 19.29.30035)
set(FMT_CAN_MODULE ON)
set(FMT_CAN_MODULE OFF)

This comment has been minimized.

Sign in to view

Copy link
@laitingsheng

laitingsheng Oct 10, 2022 •
edited
Loading

Contributor

@vitaut Honestly, I don't quite understand why this change has been committed. This change essentially disabled the module system completely regardless of the compiler because the initial value of FMT_CAN_MODULE is OFF, plus there is nowhere else change this value to enable the new module system.

If the intended behaviour is to remove the support for the MSVC module system only, I would suggest simply changing the if guard to CMAKE_CXX_STANDARD GREATER 17 AND NOT MSVC and keep set(FMT_CAN_MODULE ON).

This comment has been minimized.

Sign in to view

Copy link
@vitaut

vitaut Oct 10, 2022

Contributor

IIRC it was a workaround for broken modules in MSVC. See the PR that added it for details. It is not supposed to be changed by users, that's what FMT_MODULE is for.

This comment has been minimized.

Sign in to view

Copy link
@laitingsheng

laitingsheng Oct 11, 2022 •
edited
Loading

Contributor

I've read the PR yesterday, but I don't think this relates to any contents of the PR. The current code is

option(FMT_MODULE "Build a module instead of a traditional library." OFF)

...

set(FMT_CAN_MODULE OFF)
if (CMAKE_CXX_STANDARD GREATER 17 AND
    # msvc 16.10-pre4
    MSVC AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 19.29.30035)
  set(FMT_CAN_MODULE OFF)
endif ()
if (NOT FMT_CAN_MODULE)
  set(FMT_MODULE OFF)
  message(STATUS "Module support is disabled.")
endif ()

Since FMT_CAN_MODULE is always OFF, FMT_MODULE will always be changed to OFF regardless of the initial value. If the MSVC module is broken, then the best way to fix this (remove support for MSVC modules completely) is:

option(FMT_MODULE "Build a module instead of a traditional library." OFF)

...

set(FMT_CAN_MODULE OFF)
if (CMAKE_CXX_STANDARD GREATER 17 AND NOT MSVC)
  set(FMT_CAN_MODULE ON)
endif ()
if (NOT FMT_CAN_MODULE)
  set(FMT_MODULE OFF)
  message(STATUS "Module support is disabled.")
endif ()

Or even better, we could use cmake_dependent_option like:

set(FMT_CAN_MODULE OFF)
if (CMAKE_CXX_STANDARD GREATER 17 AND NOT MSVC)
  set(FMT_CAN_MODULE ON)
endif ()
cmake_dependent_option(FMT_MODULE "Build a module instead of a traditional library." OFF FMT_CAN_MODULE OFF)
if (NOT FMT_MODULE)
  message(STATUS "Module support is disabled.")
endif ()

This comment has been minimized.

Sign in to view

Copy link
@vitaut

vitaut Oct 11, 2022

Contributor

A PR to change this is welcome.
endif ()
if (NOT FMT_CAN_MODULE)
set(FMT_MODULE OFF)
Expand Down

0 comments on commit 3def950

Please sign in to comment.