Skip to content
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.

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

fix!: remove /FI (#22) and revert some changes in #19 #23

Merged
merged 4 commits into from
Sep 9, 2023

Conversation

gottyduke
Copy link
Contributor

@gottyduke gottyduke commented Sep 9, 2023

  1. Removed /FI force include directive flag to accommodate recent vcpkg port update. This addresses the redefinition error when compiling project with a clean commonblisf vcpkg package.
  2. Reverted some notable changes in chore: cleanup CMakePresets.json, add Ninja configs #19 , see below.

Changes reverted:
In CommonLibSF/CMakePresets.json:

{
  "name": "buildtype-debug-msvc",
  "hidden": true,
  "cacheVariables": {
    "CMAKE_BUILD_TYPE": "Debug",
    "CMAKE_CXX_FLAGS_DEBUG": "/Od /MDd",
    "CMAKE_SHARED_LINKER_FLAGS_DEBUG": "/DEBUG:FULL /LTCG:INCREMENTAL /DEBUGTYPE:FIXUP"
  }
}

Specifying /LTCG:INCREMENTAL for a debug build with /Od doesn't make sense here, /LTCG would imply /GL and /Od disables all optimizations. The other reason is that /LTCG flag is explicitly turned off for debug build with

set(CMAKE_INTERPROCEDURAL_OPTIMIZATION ON)
set(CMAKE_INTERPROCEDURAL_OPTIMIZATION_DEBUG OFF)

Then the /MDd flag is also redundant, this has been specified with

"CMAKE_MSVC_RUNTIME_LIBRARY": "MultiThreaded$<$<CONFIG:Debug>:Debug>DLL"

Othe occurrences of /LTCG and /MDd//MTd has been removed for the same reason.
/MACHINE:x64 has been removed for linker flag, this is unnecessary.

configurePresets and buildPresets have added the <config>-clang-cl-msvc variants, for generating a visual studio solution with ClangCL toolset.


Fixes added for #19 :
Updated the batch files to reflect the above CMakePresets.json changes.


closes #22

@ThirdEyeSqueegee
Copy link
Member

ThirdEyeSqueegee commented Sep 9, 2023

Should keep the /LTCG:INCREMENTAL flag for release builds. Note that as mentioned in the MSVC docs this flag is not the same as the incremental linking flag.

@gottyduke
Copy link
Contributor Author

Should keep the LTCG flag for release builds

Yes it's enabled with the same statements

set(CMAKE_INTERPROCEDURAL_OPTIMIZATION ON)
set(CMAKE_INTERPROCEDURAL_OPTIMIZATION_DEBUG OFF)

This is written in CMakeLists.txt, if you feel the need to change it, then you could still append them in the CMakePresets.json

@ThirdEyeSqueegee
Copy link
Member

I don't think the CMake command sets /LTCG:INCREMENTAL specifically?

@gottyduke
Copy link
Contributor Author

gottyduke commented Sep 9, 2023

I don't think the CMake command sets /LTCG:INCREMENTAL specifically?

It does, the IPO for MSVC is /LTCG:INCREMENTAL
I have just checked with the latest CMake implementation, it's actually /LTCG with /GL, so it might not set that.

@ThirdEyeSqueegee
Copy link
Member

Nice! TIL 😁

@gottyduke
Copy link
Contributor Author

gottyduke commented Sep 9, 2023

Correct me if I did it wrong, feel free to fix any bug if I brought them in.

Copy link
Member

@ThirdEyeSqueegee ThirdEyeSqueegee left a comment

Choose a reason for hiding this comment

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

LGTM besides the (accidental?) CMakePresets schema version change

CommonLibSF/CMakePresets.json Outdated Show resolved Hide resolved
@gottyduke
Copy link
Contributor Author

Again regarding the /LTCG:INCREMENTAL, current build only enables IPO (which is a combination of /LTCG and/GL), and previous CLib implementations don't have this specific flag on. Should we discuss more before adding this flag explicitly?

@ThirdEyeSqueegee
Copy link
Member

ThirdEyeSqueegee commented Sep 9, 2023

"For even faster performance, use /LTCG:INCREMENTAL. This option tells the linker to reoptimize only the files affected by a source file change, instead of the entire project. This option can significantly reduce the link time required. This option isn't the same option as incremental linking."

From MSVC docs. It's fine to use the flag. Also note that /LTCG is implied with /GL

EDIT: Apparently it's safer to use regular /LTCG so let's stick with that

@gottyduke gottyduke merged commit 1cd48a3 into Starfield-Reverse-Engineering:main Sep 9, 2023
3 checks passed
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.

Possibly causing redefinition errors when consuming CLibSF thru vcpkg
2 participants