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

Modernize use override #661

Closed
wants to merge 3 commits into from
Closed

Conversation

e-kwsm
Copy link
Contributor

@e-kwsm e-kwsm commented Jan 25, 2025

pugixml/CMakeLists.txt

Lines 71 to 79 in 9d7fcbf

# Set the default C++ standard to C++17 if not set; CMake will automatically downgrade this if the compiler does not support it
# When CMAKE_CXX_STANDARD_REQUIRED is set, we fall back to C++11 to avoid breaking older compilers
if (NOT DEFINED CMAKE_CXX_STANDARD_REQUIRED AND NOT DEFINED CMAKE_CXX_STANDARD AND NOT CMAKE_VERSION VERSION_LESS 3.8)
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED OFF)
elseif (NOT DEFINED CMAKE_CXX_STANDARD)
set(CMAKE_CXX_STANDARD 11)
endif()

C++11 or later is necessary, so override must be available.

@e-kwsm e-kwsm force-pushed the modernize-use-override branch from 5a71add to 25e3fd2 Compare January 25, 2025 02:36
@zeux
Copy link
Owner

zeux commented Jan 25, 2025

There seems to be some confusion in the two PRs you opened; to save time, I'll just note here that neither of them will be merged. pugixml documents its portability guarantees here https://pugixml.org/docs/manual.html#install.portability - it does not require C++11 and must be compatible with earlier versions of C++ and compilers. CMake by default uses C++11 to make sure users get access to C++11 (and now 17) features automatically, but you can override that with CMAKE_CXX_STANDARD as well.

@zeux zeux closed this Jan 25, 2025
@e-kwsm
Copy link
Contributor Author

e-kwsm commented Jan 26, 2025

Thank you for your review. I understand your point.
But virtual is unnecessary for overloaded function, which is fixed in 081e8a3.

Would you reopen this? I will revert the other commits.

@zeux
Copy link
Owner

zeux commented Jan 26, 2025

Feel free to resubmit just that commit as a separate PR.

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.

2 participants