This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Make filesystem tests not rely on libc++ internals
ClosedPublic

Authored by mstorsjo on Feb 24 2022, 5:46 AM.

Details

Summary

As part of https://reviews.llvm.org/D119036
(506cf6dc048835c598b654e43ed8f723a42e39ba), -DNOMINMAX was
dropped from the Windows CI configurations, replaced with a
block with _LIBCPP_PUSH_MACROS, #include <__undef_macros>
and _LIBCPP_POP_MACROS (and
ADDITIONAL_COMPILE_FLAGS: -DNOMINMAX left in two tests).

However, this workaround breaks the running the libc++ tests
against a different C++ standard library than libc++, as those
macros and that header are libc++ internals.

This works around it by simply doing #undef min and #undef max
(when on Windows), making the tests work on other C++ standard
libraries again.

Diff Detail

Event Timeline

mstorsjo requested review of this revision.Feb 24 2022, 5:46 AM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2022, 5:46 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested changes to this revision.Mar 1 2022, 12:30 PM
ldionne added inline comments.
libcxx/test/support/filesystem_test_helper.h
35

Instead, could we guard against those macros by doing something like (std::numeric_limits<large_file_offset_t>::max)()?

Otherwise, we could also add -DNOMINMAX in the testing config when we're on Windows. I'm not a huge fan of undef'ing those macros here in this file, it seems kind of ad-hoc.

This revision now requires changes to proceed.Mar 1 2022, 12:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 12:30 PM
ldionne added inline comments.Mar 1 2022, 12:32 PM
libcxx/test/support/filesystem_test_helper.h
35

Yeah, FWIW, I think this is where I made a mistake: https://reviews.llvm.org/D119036#3301636

I should have left -DNOMINMAX in place, at least on Windows.

mstorsjo added inline comments.Mar 1 2022, 12:39 PM
libcxx/test/support/filesystem_test_helper.h
35

Yeah I don't mind putting that define back.

mstorsjo updated this revision to Diff 412205.Mar 1 2022, 12:47 PM

Reinstate -DNOMINMAX instead of using libc++ specifics in filesystem_test_helper.h.

ldionne accepted this revision.Mar 1 2022, 1:12 PM
This revision is now accepted and ready to land.Mar 1 2022, 1:12 PM
mstorsjo updated this revision to Diff 412257.Mar 1 2022, 2:39 PM

Add an extra -Wno-macro-redefined in a test that includes libcxx internal headers (which also define NOMINMAX). Will push later if CI is passing.