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

Add read-core feature gate #596

Merged
merged 2 commits into from
Oct 13, 2021
Merged

Add read-core feature gate #596

merged 2 commits into from
Oct 13, 2021

Conversation

nbdd0121
Copy link
Contributor

Fix #581

src/read/unit.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@nbdd0121
Copy link
Contributor Author

It seems that

#[derive(Debug)]
struct Foo<#[cfg(foo)] T, #[cfg(not(foo))] T>(T);

only works in Rust 1.48+. However it doesn't seem to be mentioned in the release notes.

@nbdd0121
Copy link
Contributor Author

nbdd0121 commented Oct 12, 2021

Bisection shows that it's changed in rust-lang/rust#76010, which fixes rust-lang/rust#75930. I could manually implement these traits, or perhaps we can bump MSRV to 1.48.

@nbdd0121 nbdd0121 changed the title Add read-no-alloc feature gate Add read-core feature gate Oct 12, 2021
@philipc
Copy link
Collaborator

philipc commented Oct 13, 2021

When does the type checking for type parameters occur? It seems defaulting to StoreOnHeap always compiles, even if StoreOnHeap is missing the storage implementation? If so, it's not ideal to have a default that won't work, but for working code it makes no difference.

Otherwise, 1.48 is okay, it's getting close to a year old.

@nbdd0121
Copy link
Contributor Author

It seems that the checking is lazy if the bound depends on generic parameter:

trait Bar<U> {}

struct Foo<U, T: Bar<U> = ()>(U, T);
//^ this is lazy, since () may implement Bar<U> for some U, but not necessary all Us

and this is eagerly checked:

trait Bar {}

struct Foo<T: Bar = ()>(T);
//^ this is eager, so will raise an error

src/constants.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@philipc philipc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@philipc philipc merged commit 7a3aeef into gimli-rs:master Oct 13, 2021
@philipc
Copy link
Collaborator

philipc commented Oct 13, 2021

Does this all look good for a new release now?

@nbdd0121
Copy link
Contributor Author

No PRs left to submit from my side!

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.

Use gimli without alloc
2 participants