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

Ignoring test case: [codegen] repr-transparent-aggregates-1.rs for aarch64 #73926

Merged
merged 1 commit into from
Jul 16, 2020
Merged

Ignoring test case: [codegen] repr-transparent-aggregates-1.rs for aarch64 #73926

merged 1 commit into from
Jul 16, 2020

Conversation

joaopaulocarreiro
Copy link
Contributor

Ignoring test case: [codegen] repr-transparent-aggregates-1.rs for aarch64.

Copyright (c) 2020, Arm Limited.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 1, 2020
@nikomatsakis
Copy link
Contributor

I don't have any objection to ignoring this test -- it looks like a lot of other architectures do so -- but I'm curious what's motivating this PR =)

@hanna-kruppe
Copy link
Contributor

Background for this test file and its twins:

// All that remains to be tested are aggregates. They are tested in separate files called repr-
// transparent-*.rs with `only-*` or `ignore-*` directives, because the expected LLVM IR
// function signatures vary so much that it's not reasonably possible to cover all of them with a
// single CHECK line.
//
// You may be wondering why we don't just compare the return types and argument types for equality
// with FileCheck regex captures. Well, rustc doesn't perform newtype unwrapping on newtypes
// containing aggregates. This is OK on all ABIs we support, but because LLVM has not gotten rid of
// pointee types yet, the IR function signature will be syntactically different (%Foo* vs
// %FooWrapper*).

So, it's perfectly fine to ignore this test on AArch64 if the ABI lowering there is different from what's tested in that file, but ideally we'd have a test of this sort for each target (the existing repr-transparent-{1,2,3}.rs tests already cover quite a few targets), so just ignoring is not great IMO.

@joaopaulocarreiro
Copy link
Contributor Author

joaopaulocarreiro commented Jul 2, 2020

The motivation behind the "just ignoring" approach for this PR was based on the fact that I was looking at this as two independent tasks. One of them is to ignore this test for aarch64 (inevitable) and the other is to add suitable tests for the same target. The former is immediate, the latter requires a bit more effort, and is something that should follow.

But if we should proceed in a different way, that's fine too.

@nikomatsakis
Copy link
Contributor

@joaopaulocarreiro can you file a follow-up issue regarding adding the new tests and then assign it to yourself? (via @rustbot claim)

Sorry for being slow to answer, I've fallen behind on notifications and reviews this week.

@nikomatsakis
Copy link
Contributor

@bors delegate

Once that is done, you can feel free to write @bors r=nikomatsakis to merge this PR.

@nikomatsakis
Copy link
Contributor

@bors delegate+

@bors
Copy link
Contributor

bors commented Jul 9, 2020

✌️ @joaopaulocarreiro can now approve this pull request

@nikomatsakis
Copy link
Contributor

I created #74396 and @joaopaulocarreiro took the liberty of assigning you (you can release assignment if you would like).

@bors r+

@bors
Copy link
Contributor

bors commented Jul 16, 2020

📌 Commit 582071c has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 16, 2020
@nikomatsakis
Copy link
Contributor

@bors rollup

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 16, 2020
…nikomatsakis

Ignoring test case: [codegen] repr-transparent-aggregates-1.rs for aarch64

Ignoring test case: [codegen] repr-transparent-aggregates-1.rs for aarch64.

Copyright (c) 2020, Arm Limited.
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 16, 2020
…nikomatsakis

Ignoring test case: [codegen] repr-transparent-aggregates-1.rs for aarch64

Ignoring test case: [codegen] repr-transparent-aggregates-1.rs for aarch64.

Copyright (c) 2020, Arm Limited.
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 16, 2020
…nikomatsakis

Ignoring test case: [codegen] repr-transparent-aggregates-1.rs for aarch64

Ignoring test case: [codegen] repr-transparent-aggregates-1.rs for aarch64.

Copyright (c) 2020, Arm Limited.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 16, 2020
…arth

Rollup of 21 pull requests

Successful merges:

 - rust-lang#73566 (Don't run `everybody_loops` for rustdoc; instead ignore resolution errors)
 - rust-lang#73771 (Don't pollute docs/suggestions with libstd deps)
 - rust-lang#73794 (Small cleanup for E0705 explanation)
 - rust-lang#73807 (rustdoc: glue tokens before highlighting)
 - rust-lang#73835 (Clean up E0710 explanation)
 - rust-lang#73926 (Ignoring test case: [codegen] repr-transparent-aggregates-1.rs for aarch64)
 - rust-lang#73981 (Remove some `ignore-stage1` annotations.)
 - rust-lang#73998 (add regression test for rust-lang#61216)
 - rust-lang#74140 (Make hir ProjectionKind more precise)
 - rust-lang#74148 (Move #[doc(alias)] check in rustc)
 - rust-lang#74159 (forbid generic params in the type of const params)
 - rust-lang#74171 (Fix 44056 test with debug on macos.)
 - rust-lang#74221 (Don't panic if the lhs of a div by zero is not statically known)
 - rust-lang#74325 (Focus on the current file in the source file sidebar)
 - rust-lang#74359 (rustdoc: Rename internal API fns to `into_string`)
 - rust-lang#74370 (Reintroduce spotlight / "important traits" feature)
 - rust-lang#74390 (Fix typo in std::mem::transmute documentation)
 - rust-lang#74391 (BtreeMap: superficially refactor root access)
 - rust-lang#74392 (const generics triage)
 - rust-lang#74397 (Fix typo in the latest release note)
 - rust-lang#74406 (Set shell for github actions CI)

Failed merges:

r? @ghost
@bors bors merged commit 622a8b8 into rust-lang:master Jul 16, 2020
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants