This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Explain an XFAIL LIBCXX-WINDOWS-FIXME and convert into "UNSUPPORTED"
ClosedPublic

Authored by mstorsjo on May 12 2021, 12:45 PM.

Download Raw Diff

Details

Reviewers

curdeius
Mordante

Group Reviewers

Restricted Project

Commits

rGec402f1d5da0: [libcxx] [test] Explain an XFAIL LIBCXX-WINDOWS-FIXME and convert into…

Summary

This particular test relies on internal details from the libc++
filesystem implementation header, and those details are structured
differently in the implementation for windows.

Diff Detail

Repository: rG LLVM Github Monorepo

Event Timeline

mstorsjo requested review of this revision.May 12 2021, 12:45 PM

mstorsjo created this revision.

Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2021, 12:45 PM

Herald added a reviewer: Restricted Project. · View Herald Transcript

Harbormaster completed remote builds in B104113: Diff 344917.May 12 2021, 2:26 PM

Rerun CI

curdeius added a subscriber: curdeius.May 13 2021, 10:14 AM

curdeius added inline comments.

libcxx/test/libcxx/input.output/filesystems/class.directory_entry/directory_entry.mods/last_write_time.pass.cpp
16	Your patch description said that it was going to be converted into UNSUPPORTED, right?

mstorsjo added inline comments.May 13 2021, 10:53 AM

libcxx/test/libcxx/input.output/filesystems/class.directory_entry/directory_entry.mods/last_write_time.pass.cpp
16	Oh, right. What do you think, which marking would be better here?

Harbormaster completed remote builds in B104294: Diff 345150.May 13 2021, 10:54 AM

curdeius accepted this revision as: curdeius.May 14 2021, 8:28 AM

curdeius added inline comments.

libcxx/test/libcxx/input.output/filesystems/class.directory_entry/directory_entry.mods/last_write_time.pass.cpp
16	No strong feelings here. I'd probably go for unsupported. Your comment clearly explains why it is so IMO. Also, unless you think it will change in a foreseeable future, it makes no sense to me to XFAIL. But, yeah, I leave it to you (and other reviewers).

Ping

Some small nits other than that LGTM. The UNSUPPORTED change shouldn't break anything, but please keep eye out. Alternatively give it another CI run and land it after the build passes.

libcxx/test/libcxx/input.output/filesystems/class.directory_entry/directory_entry.mods/last_write_time.pass.cpp
13	Please capitalize Windows.
16	Reading the comment I think UNSUPPORTED would be better.

This revision is now accepted and ready to land.May 25 2021, 9:58 AM

Closed by commit rGec402f1d5da0: [libcxx] [test] Explain an XFAIL LIBCXX-WINDOWS-FIXME and convert into… (authored by mstorsjo). · Explain WhyMay 25 2021, 1:05 PM

This revision was automatically updated to reflect the committed changes.

mstorsjo added a commit: rGec402f1d5da0: [libcxx] [test] Explain an XFAIL LIBCXX-WINDOWS-FIXME and convert into….

Revision Contents

Path

Size

libcxx/

test/

libcxx/

input.output/

filesystems/

class.directory_entry/

directory_entry.mods/

last_write_time.pass.cpp

6 lines

Diff 347763

libcxx/test/libcxx/input.output/filesystems/class.directory_entry/directory_entry.mods/last_write_time.pass.cpp

	//===----------------------------------------------------------------------===//			//===----------------------------------------------------------------------===//
	//			//
	// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.			// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
	// See https://llvm.org/LICENSE.txt for license information.			// See https://llvm.org/LICENSE.txt for license information.
	// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception			// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
	//			//
	//===----------------------------------------------------------------------===//			//===----------------------------------------------------------------------===//

	// UNSUPPORTED: c++03			// UNSUPPORTED: c++03
	// ADDITIONAL_COMPILE_FLAGS: -I %S/../../../../../../src/filesystem			// ADDITIONAL_COMPILE_FLAGS: -I %S/../../../../../../src/filesystem

	// XFAIL: LIBCXX-WINDOWS-FIXME			// This test relies on calling functions from the libcxx internal header
				// filesystem_common.h; the Windows implementation uses different
				MordanteUnsubmitted Not Done Reply Inline Actions Please capitalize Windows. Mordante: Please capitalize Windows.
				// internals and doesn't provide the same set_file_times function as for
				// other platforms.
				// UNSUPPORTED: windows
				curdeiusUnsubmitted Not Done Reply Inline Actions Your patch description said that it was going to be converted into UNSUPPORTED, right? curdeius: Your patch description said that it was going to be converted into UNSUPPORTED, right?
				mstorsjoAuthorUnsubmitted Done Reply Inline Actions Oh, right. What do you think, which marking would be better here? mstorsjo: Oh, right. What do you think, which marking would be better here?
				curdeiusUnsubmitted Not Done Reply Inline Actions No strong feelings here. I'd probably go for unsupported. Your comment clearly explains why it is so IMO. Also, unless you think it will change in a foreseeable future, it makes no sense to me to XFAIL. But, yeah, I leave it to you (and other reviewers). curdeius: No strong feelings here. I'd probably go for unsupported. Your comment clearly explains why it…
				MordanteUnsubmitted Not Done Reply Inline Actions Reading the comment I think UNSUPPORTED would be better. Mordante: Reading the comment I think UNSUPPORTED would be better.

	// <filesystem>			// <filesystem>

	// class directory_entry			// class directory_entry

	#include "filesystem_include.h"			#include "filesystem_include.h"
	#include <type_traits>			#include <type_traits>
	#include <cassert>			#include <cassert>
	▲ Show 20 Lines • Show All 72 Lines • Show Last 20 Lines