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

chore: mark TypeInfo derived impl as automatically_derived #218

Merged
merged 4 commits into from
Feb 19, 2025

Conversation

saiintbrisson
Copy link
Contributor

As per rust-lang/rust#120185, functions with their enclosing impl body marked as #[automatically_derived] won't be featured in coverage reports.

An example:
Screenshot 2025-02-14 at 08 05 07

As per rust-lang/rust#120185, functions with their enclosing impl body marked as `#[automatically_derived]` won't be featured in coverage reports.
@niklasad1 niklasad1 requested a review from pkhry February 14, 2025 11:18
@saiintbrisson
Copy link
Contributor Author

@niklasad1 fixed the lifetimes clippy complained about

@saiintbrisson
Copy link
Contributor Author

It keeps failing for other reasons, ill look it up later and fix everything

paritytech/parity-scale-codec/pull/653 disallowed duplicate indexes for variants.
@saiintbrisson
Copy link
Contributor Author

@niklasad1 hi again, paritytech/parity-scale-codec#653 disallowed duplicate indexes for variants. I switched the numbers and it should be good to go. I made this PR through the file editor in GitHub so I haven't tested it.

@niklasad1
Copy link
Member

hey @saiintbrisson again sorry about the CI mess that you are running into

The start attribute has been removed and can be replaced with the following patch:

diff --git a/test_suite/derive_tests_no_std/src/main.rs b/test_suite/derive_tests_no_std/src/main.rs
index a5af8c4..ae09da5 100644
--- a/test_suite/derive_tests_no_std/src/main.rs
+++ b/test_suite/derive_tests_no_std/src/main.rs
@@ -12,12 +12,11 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 #![allow(internal_features)]
-#![feature(lang_items, start)]
-#![feature(alloc_error_handler)]
+#![feature(lang_items, alloc_error_handler)]
 #![no_std]
+#![no_main]
 
-#[start]
-fn start(_argc: isize, _argv: *const *const u8) -> isize {
+pub extern "C" fn _start(_argc: isize, _argv: *const *const u8) -> isize {
     test();
     0
 }

@saiintbrisson
Copy link
Contributor Author

hey @saiintbrisson again sorry about the CI mess that you are running into

The start attribute has been removed and can be replaced with the following patch:

diff --git a/test_suite/derive_tests_no_std/src/main.rs b/test_suite/derive_tests_no_std/src/main.rs
index a5af8c4..ae09da5 100644
--- a/test_suite/derive_tests_no_std/src/main.rs
+++ b/test_suite/derive_tests_no_std/src/main.rs
@@ -12,12 +12,11 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 #![allow(internal_features)]
-#![feature(lang_items, start)]
-#![feature(alloc_error_handler)]
+#![feature(lang_items, alloc_error_handler)]
 #![no_std]
+#![no_main]
 
-#[start]
-fn start(_argc: isize, _argv: *const *const u8) -> isize {
+pub extern "C" fn _start(_argc: isize, _argv: *const *const u8) -> isize {
     test();
     0
 }

I don't think this is as simple. I'll take a look into it a bit later. _start does not receive args as it is the first to run, before __libc_start_main, and it requires targeting x86_64-unknown-none instead of linux-gnu, on the contrary the _start will be duplicated.

Does the code really require no_main for this test?

@niklasad1
Copy link
Member

Does the code really require no_main for this test?

it might be wrong my bad but I think the test/check is to make sure that it runs/compiles for no-std

@niklasad1
Copy link
Member

anyway, it's unrelated to this PR let's merge this and fix it another PR

@niklasad1 niklasad1 merged commit 7629a7f into paritytech:master Feb 19, 2025
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants