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

msvc : silence codecvt c++17 deprecation warnings #8395

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

iboB
Copy link
Contributor

@iboB iboB commented Jul 9, 2024

codecvt_utf8 is deprecated in C++17 (and removed in C++26). This silences the warnings when compiling with MSVC

@ggerganov
Copy link
Owner

I thought that setting the standard to C++11 would make the compiler ignore such warnings:

target_compile_features (${TARGET} PUBLIC cxx_std_11)

Is it not the case?

@iboB
Copy link
Contributor Author

iboB commented Jul 9, 2024

I thought that setting the standard to C++11 would make the compiler ignore such warnings:

target_compile_features (${TARGET} PUBLIC cxx_std_11)

Is it not the case?

Yes, but for there may be builds which set the standard to 17 or higher from above (for ABI compatibility for example)

PS pr for C++20 compatibility incoming 😄

@ggerganov
Copy link
Owner

You only need the change in common.cpp. For src/unicode.cpp there are no ABI compatibility concerns because it does not have public C++ interface as it is part of the llama library

@slaren
Copy link
Collaborator

slaren commented Jul 9, 2024

How are you changing the C++ standard of the common library? Is that possible without changing its CMakeLists.txt file?

@iboB
Copy link
Contributor Author

iboB commented Jul 9, 2024

How are you changing the C++ standard of the common library? Is that possible without changing its CMakeLists.txt file?

If you set CMAKE_CXX_STANDARD it overrides target-specific ones for the sake of ABI compatibility

@iboB
Copy link
Contributor Author

iboB commented Jul 9, 2024

How are you changing the C++ standard of the common library? Is that possible without changing its CMakeLists.txt file?

If you set CMAKE_CXX_STANDARD it overrides target-specific ones for the sake of ABI compatibility

That is when you use cxx_std_11

@iboB
Copy link
Contributor Author

iboB commented Jul 9, 2024

You only need the change in common.cpp. For src/unicode.cpp there are no ABI compatibility concerns because it does not have public C++ interface as it is part of the llama library

Setting the standard from above affects all libs. Plus if you use static linking, it doesn't matter whether you export symbols explicity to risk ODR violations because of abi compatibility

@ggerganov ggerganov merged commit 7a80710 into ggerganov:master Jul 10, 2024
53 checks passed
Nexesenex pushed a commit to Nexesenex/croco.cpp that referenced this pull request Jul 11, 2024
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Jul 13, 2024
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Jul 13, 2024
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.

3 participants