-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Include <cmeow> instead of <meow.h>, Part2 #2852
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love that the namespaces are changed in this PR, given it's unnecessary AFAIK; additionally, I'm personally at least very much against using std::
to access C functions and types.
oops, I already updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good. I've verified that everything that should be changed, is being changed with no missed occurrences, except for the exclusions as noted in the PR description.
As mentioned in a comment, I'm nervous about changing the header units test here - I think we should go back to including the UCRT headers, with added comments.
My last concern is that this is an extremely large PR in terms of the number of files being edited, so we should try to be extra clean about "nice to have" changes that are not strictly necessary for the central intent of the PR. There are a small number of namespace changes, and two of those files were missing changes, which is easily missed in such a large PR.
As much as I love using namespace std;
in tests (I prefer for all tests in tests/std
to use using-directives, unless they are one of the unusual ones that is specifically concerned with namespaces like the UDL test), and while we could add additional changes to patch the affected files, I would strongly prefer to remove these changes from the PR so that it's completely focused on the <cmeow>
transformation - easily verified with git diff -U0
and grep
filtering.
I've verified and will push changes. With this, I am confident that the PR shouldn't disrupt tests.
@strega-nil-ms FYI, I pushed changes after you approved. |
@StephanTLavavej's changes LGTM! Thanks. |
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
cthanks cfor cthis cconsistency cimprovement! 😹 🎉 🤪 |
Co-authored-by: Stephan T. Lavavej <[email protected]>
While reviewing #2851 I found that
tests/std/tests/Dev11_0836436_get_time/test.cpp
includes#include <assert.h>
and other .h files.I recall that we already replaced
<meow.h>
to<cmeow>
. I found the PR: #1405There are still some places where we include
<meow.h>
:tr1 tests
benchmarks\inc\xoshiro.hpp
(becauseWritten in 2018 by David Blackman and Sebastiano Vigna ([email protected])
)__msvc_all_public_headers.hpp
<cmeow>
tests\std\include\force_include.hpp
(because Part of the LLVM Project)tests with
// LLVM SOURCES BEGIN
(tests\std\tests\P0088R3_variant\test.cpp
for example)