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

Remove other uses of /Zc:__cplusplus in the documentation. #3557

Merged
merged 2 commits into from Nov 22, 2023

Conversation

rcurtin
Copy link
Member

@rcurtin rcurtin commented Oct 29, 2023

This is just a follow-up to #3555, since that option is no longer required for MSVC.

@conradsnicta
Copy link
Contributor

conradsnicta commented Oct 30, 2023

There's a small fly in the ointment. Perhaps merge this only just before (or just after) the release of mlpack 4.2.2.

mlpack 4.2.1 does not have the updated C++17 detection under MSVC. If people download mlpack 4.2.1 (from mlpack.org) and then follow the updated instructions (doc/user/build_windows.md and doc/user/sample_ml_app.md), mlpack 4.2.1 is not going to work for them under MSVC.

Right now the current documentation works for both mlpack 4.2.1 and the git master branch. Use of the _MSVC_LANG macro (in #3555) can be thought of as a workaround for the bizarre and confusing config options to enable C++17 under MSVC. The workaround is there to make people's life easier if and when they confused by MSVC weirdness. Suggest to keep the current documentation to cover the changeover period between mlpack 4.2.1 and 4.2.2.

@rcurtin
Copy link
Member Author

rcurtin commented Nov 1, 2023

Yes, true, although the documentation distributed with 4.2.1 will be accurate. (It is only inaccurate if a user comes to Github to read documentation, which is probably the most common use case realistically like you pointed out.) I don't mind releasing 4.2.2 immediately after merging this, it's easy enough and minimizes the period of inaccurate documentation for older versions.

Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

@rcurtin
Copy link
Member Author

rcurtin commented Nov 7, 2023

#3547 is so close that I'm going to wait for that to be merged to merge this, and then open a release PR.

@rcurtin rcurtin merged commit 751ba46 into mlpack:master Nov 22, 2023
19 checks passed
@rcurtin rcurtin deleted the additional-cxx17-msvc-doc-fix branch November 22, 2023 13:48
@rcurtin rcurtin mentioned this pull request Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants