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

Create a serde1-alloc feature flag #111

Closed

Conversation

TethysSvensson
Copy link
Contributor

@TethysSvensson TethysSvensson commented Jul 10, 2022

Currently the bstring_serde module is hidden behind the serde1 feature flag, however this flag also pulls in the std flag, which is unnecessary.

This PR adds an additional feature flag to enable that impl without depending on the entire std flag.

Alternatives considered

  • Change the serde1 feature flag to only depend on ["alloc", "serde1-nostd", "serde/alloc"]. I do not know if you consider this backwards incompatible (or if you care).
  • Simplify the feature flags by bumping the MSRV to 1.60 and using the new cargo features.

@TethysSvensson TethysSvensson changed the title Loosen some of the features flags a bit Create a serde1-alloc feature flag Jul 10, 2022
@BurntSushi
Copy link
Owner

Hmmm yeah, good catch. I think this is the right path. To respond to your alternatives:

Change the serde1 feature flag to only depend on ["alloc", "serde1-nostd", "serde/alloc"]. I do not know if you consider this backwards incompatible (or if you care).

I'm just about to release 1.0, so this is the time to make backwards incompatible changes.

However, I think making serde1 depend on alloc like this probably doesn't work. Because then the question arises: how do you enable serde/std? I can't put it in as a dependency of the std feature, because then that will make serde an unconditional dependency on bstr whenever std is enabled. That's no good.

Simplify the feature flags by bumping the MSRV to 1.60 and using the new cargo features.

Yeah I think Rust 1.60 is just too new. This is a little unfortunate because I suspect moving to this will be a breaking change and I do not expect to release a bstr 2.0 any time soon (if ever).

@TethysSvensson
Copy link
Contributor Author

how do you enable serde/std?

Since none of the bstr code depends on serde/std feature, one option would be to punt the problem to the user: If they require the serde/std feature they have to enable it themselves.

@BurntSushi
Copy link
Owner

That is an option, but one that seems higher risk with respect to backcompat. e.g., If bstr ever does care about serde's std support.

@TethysSvensson
Copy link
Contributor Author

Then perhaps exposing three features like in this PR might be the best option?

Though if you don't care about breaking changes right now, then they could be renamed to serde-std, serde-alloc and serde-nostd?

@BurntSushi
Copy link
Owner

Right, that's what I plan to do: #40 (comment)

But serde1-core instead of serde1-nostd, since nostd can refer to either alloc-only or core-only.

Sorry if I wasn't clear about that. I was mostly just trying to write out a justification for why the alternatives were considered, but ultimately that 3 different features seems like the right way to go.

BurntSushi pushed a commit that referenced this pull request Jul 11, 2022
Now that bstr has an 'alloc' feature, we need to rethink how we setup
the serde feature flags. Previously, all we had was 'std' and 'no std'.
But now we have 'std', 'alloc only' and 'core only'. In particular, 'no
std' is split into 'alloc only' and 'core only', since neither one bring
in std. To reflect this trichotomy, we rename 'serde1' to 'serde1-std',
and split 'serde1-nostd' into 'serde1-alloc' and 'serde1-core'.

Closes #111
@TethysSvensson
Copy link
Contributor Author

Since you are renaming anyways, why not get rid of the 1?

@BurntSushi
Copy link
Owner

In case serde has a major version bump.

BurntSushi pushed a commit that referenced this pull request Jul 11, 2022
Now that bstr has an 'alloc' feature, we need to rethink how we setup
the serde feature flags. Previously, all we had was 'std' and 'no std'.
But now we have 'std', 'alloc only' and 'core only'. In particular, 'no
std' is split into 'alloc only' and 'core only', since neither one bring
in std. To reflect this trichotomy, we rename 'serde1' to 'serde1-std',
and split 'serde1-nostd' into 'serde1-alloc' and 'serde1-core'.

Closes #111
@BurntSushi BurntSushi mentioned this pull request Jul 11, 2022
@TethysSvensson TethysSvensson deleted the feature-flags branch July 11, 2022 23:13
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.

2 participants