This is an archive of the discontinued LLVM Phabricator instance.

Paths

Table of Contentst

-
libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/
-
test/
-
std/
-
utilities/
-
function.objects/
-
func.wrap/
-
func.wrap.func/
-
func.wrap.func.con/
2/2
copy_move.pass.cpp

Differential D155760

[libcxx] Fix "copy_move.pass" test
ClosedPublic

Authored by haowei on Jul 19 2023, 3:46 PM.

Download Raw Diff

Details

Reviewers

mstorsjo
phosek
ldionne

Group Reviewers

Restricted Project

Commits

rG310255abef23: [libcxx] Fix copy_move.pass test
rGbe9f55f4fff4: [libcxx] Fix copy_move.pass test

Summary

When LLVM is built under MSVC and libcxx ABI is set to 2, the 'copy_move.pass' test will unexpectedly pass. This patch mitigate this issue by setting this test will only expecting FAIL when libcxx ABI version is set to 1.

Diff Detail

Repository: rG LLVM Github Monorepo

Event Timeline

haowei created this revision.Jul 19 2023, 3:46 PM

Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2023, 3:46 PM

Tested under Windows with LIT_FILTER=.*func.wrap.func.con/copy_move.pass.cpp ninja check-runtimes-x86_64-pc-windows-msvc . The unexpected pass issue disappeared.

LGTM

This revision is now accepted and ready to land.Jul 19 2023, 3:48 PM

This revision was landed with ongoing or failed builds.Jul 19 2023, 3:52 PM

Closed by commit rGbe9f55f4fff4: [libcxx] Fix copy_move.pass test (authored by haowei). · Explain Why

This revision was automatically updated to reflect the committed changes.

haowei added a commit: rGbe9f55f4fff4: [libcxx] Fix copy_move.pass test.

Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2023, 3:52 PM

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

Herald added a subscriber: libcxx-commits. · View Herald Transcript

ldionne added a reverting change: rGb9a599cfe203: [libc++] Revert "[libcxx] Fix copy_move.pass test".Jul 19 2023, 4:11 PM

I reverted this change. As a matter of policy, please do not commit patches to libc++ unless the libc++ review group is green. The review process we follow is documented here: https://libcxx.llvm.org/Contributing.html#the-review-process -- it is a bit stricter than the usual LLVM review process because we have an automatically-added review group.

Furthermore, the change itself looked wrong, see my comment.

libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/copy_move.pass.cpp
11	`libcxx-abi-version-1` is not a Lit feature that we ever populate. We do have `libcpp-abi-version=<VERSION>`, so this should probably be `libcpp-abi-version=1`.

In D155760#4516664, @ldionne wrote:

I reverted this change. As a matter of policy, please do not commit patches to libc++ unless the libc++ review group is green. The review process we follow is documented here: https://libcxx.llvm.org/Contributing.html#the-review-process -- it is a bit stricter than the usual LLVM review process because we have an automatically-added review group.

Furthermore, the change itself looked wrong, see my comment.

OK, I will pay attention next time.

I see why my patch works in the local test now.
That tag wasn't defined.

I corrected the issue. @ldionne Could you take a look. Thanks.

libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/copy_move.pass.cpp
11	I corrected this tag.

LGTM if CI is green.

This revision is now accepted and ready to land.Jul 19 2023, 4:30 PM

In D155760#4516670, @haowei wrote:

In D155760#4516664, @ldionne wrote:

I reverted this change. As a matter of policy, please do not commit patches to libc++ unless the libc++ review group is green. The review process we follow is documented here: https://libcxx.llvm.org/Contributing.html#the-review-process -- it is a bit stricter than the usual LLVM review process because we have an automatically-added review group.

Furthermore, the change itself looked wrong, see my comment.

OK, I will pay attention next time.

I see why my patch works in the local test now.
That tag wasn't defined.

No worries, you'll know for next time!

This revision was landed with ongoing or failed builds.Jul 19 2023, 4:53 PM

Closed by commit rG310255abef23: [libcxx] Fix copy_move.pass test (authored by haowei). · Explain Why

This revision was automatically updated to reflect the committed changes.

haowei marked an inline comment as done.

haowei added a commit: rG310255abef23: [libcxx] Fix copy_move.pass test.

In D155760#4516691, @ldionne wrote:

LGTM if CI is green.

Just FWIW, it seems like the premerge CI never ran for this change. As long as all is well and it didn't trip anything up, this was fine, but ideally we should have fixed (setting the "repository" field to "rG" in the phabricator upload form, which gets set automatically when posting with arcanist) that and made sure the CI actually ran, before pushing it. It looks like the later scheduled runs on main are fine though.

In D155760#4517369, @mstorsjo wrote:

In D155760#4516691, @ldionne wrote:

LGTM if CI is green.

Just FWIW, it seems like the premerge CI never ran for this change. As long as all is well and it didn't trip anything up, this was fine, but ideally we should have fixed (setting the "repository" field to "rG" in the phabricator upload form, which gets set automatically when posting with arcanist) that and made sure the CI actually ran, before pushing it. It looks like the later scheduled runs on main are fine though.

I locally test it before I submit the change.

I noticed that the premerge bot unexpected timed out when checking out this change and I don't know how to kick it start again. So I just landed it and monitor the post submit bots.

In D155760#4517443, @haowei wrote:

In D155760#4517369, @mstorsjo wrote:

In D155760#4516691, @ldionne wrote:

LGTM if CI is green.

Just FWIW, it seems like the premerge CI never ran for this change. As long as all is well and it didn't trip anything up, this was fine, but ideally we should have fixed (setting the "repository" field to "rG" in the phabricator upload form, which gets set automatically when posting with arcanist) that and made sure the CI actually ran, before pushing it. It looks like the later scheduled runs on main are fine though.

I noticed that the premerge bot unexpected timed out when checking out this change and I don't know how to kick it start again. So I just landed it and monitor the post submit bots.

We don't have post-submit bots in libc++. Please make sure to follow these instructions for contributing in the future: https://libcxx.llvm.org/Contributing.html. In particular, using arc diff to upload your patch is mandatory for libc++, since it will ensure that CI runs properly.

I locally test it before I submit the change.

We test 58 configurations in the CI. Local testing is not close to being enough for libc++.

In D155760#4519028, @ldionne wrote:

In D155760#4517443, @haowei wrote:

In D155760#4517369, @mstorsjo wrote:

In D155760#4516691, @ldionne wrote:

LGTM if CI is green.

Just FWIW, it seems like the premerge CI never ran for this change. As long as all is well and it didn't trip anything up, this was fine, but ideally we should have fixed (setting the "repository" field to "rG" in the phabricator upload form, which gets set automatically when posting with arcanist) that and made sure the CI actually ran, before pushing it. It looks like the later scheduled runs on main are fine though.

I noticed that the premerge bot unexpected timed out when checking out this change and I don't know how to kick it start again. So I just landed it and monitor the post submit bots.

We don't have post-submit bots in libc++. Please make sure to follow these instructions for contributing in the future: https://libcxx.llvm.org/Contributing.html. In particular, using arc diff to upload your patch is mandatory for libc++, since it will ensure that CI runs properly.

I locally test it before I submit the change.

We test 58 configurations in the CI. Local testing is not close to being enough for libc++.

I see. I will follow the instruction next time. Sorry about that.

Revision Contents

Path

Size

libcxx/

test/

std/

utilities/

function.objects/

func.wrap/

func.wrap.func/

func.wrap.func.con/

copy_move.pass.cpp

2 lines

Diff 542247

libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.con/copy_move.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
	//			//
	//===----------------------------------------------------------------------===//			//===----------------------------------------------------------------------===//

	// FIXME: In MSVC mode, even "std::function<int(int)> f(aref);" causes			// FIXME: In MSVC mode, even "std::function<int(int)> f(aref);" causes
	// allocations.			// allocations.
	// XFAIL: target=x86_64-pc-windows-msvc && stdlib=libc++			// XFAIL: target=x86_64-pc-windows-msvc && stdlib=libc++ && libcpp-abi-version=1
				ldionneUnsubmitted Done Reply Inline Actions `libcxx-abi-version-1` is not a Lit feature that we ever populate. We do have `libcpp-abi-version=<VERSION>`, so this should probably be `libcpp-abi-version=1`. ldionne: `libcxx-abi-version-1` is not a Lit feature that we ever populate. We do have `libcpp-abi…
				haoweiAuthorUnsubmitted Done Reply Inline Actions I corrected this tag. haowei: I corrected this tag.

	// UNSUPPORTED: c++03			// UNSUPPORTED: c++03

	// <functional>			// <functional>

	// class function<R(ArgTypes...)>			// class function<R(ArgTypes...)>

	// function(const function& f);			// function(const function& f);
	▲ Show 20 Lines • Show All 159 Lines • Show Last 20 Lines