Skip to content

mark Ascii::new(), UniCase::unicode() as const#28

Merged
seanmonstar merged 3 commits intoseanmonstar:masterfrom
abonander:master
Mar 4, 2019
Merged

mark Ascii::new(), UniCase::unicode() as const#28
seanmonstar merged 3 commits intoseanmonstar:masterfrom
abonander:master

Conversation

@abonander
Copy link
Copy Markdown
Contributor

closes #27

Comment thread src/lib.rs
impl<S> UniCase<S> {
/// Creates a new `UniCase`, skipping the ASCII check.
pub fn unicode(s: S) -> UniCase<S> {
pub const fn unicode(s: S) -> UniCase<S> {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

const fn can only currently reside in impl blocks without trait bounds.

@abonander
Copy link
Copy Markdown
Contributor Author

Oh man, minimum version of Rust is 1.3. Is that a hard requirement?

@abonander
Copy link
Copy Markdown
Contributor Author

We could put this behind a Cargo feature instead.

@abonander
Copy link
Copy Markdown
Contributor Author

@seanmonstar should I fix this with another version check/cfg in build.rs?

@seanmonstar
Copy link
Copy Markdown
Owner

Oh man, minimum version of Rust is 1.3. Is that a hard requirement?

Oh wow, that's old! I'm sure I set up CI with that back then, and never really needed anything newer...

I don't think we need to support 1.3 forever, but since this requires 1.32, that's likely too new to not be a breaking change. Is it possible to work with a config in the build.rs? Or does Rust 1.3 hit a parse error before evaluating the config flag?

@abonander
Copy link
Copy Markdown
Contributor Author

I'm trying it now. By the way, I'm a bit concerned by the .unwrap_or(true)s on the version check... if we can't determine the version then shouldn't we not assume these features are available?

@abonander
Copy link
Copy Markdown
Contributor Author

If it does cause parse errors I could probably put it in a separate file and #[cfg] that. I don't think non-cfg'd modules are even loaded.

@abonander
Copy link
Copy Markdown
Contributor Author

1.3.0 doesn't want to build this locally, it fails trying to invoke link.exe, so I'll let Travis test it. I tried 1.30.0 and it worked fine but I think parsing for const fn was already implemented. I might try 1.15.0, I think that's pretty old.

@abonander
Copy link
Copy Markdown
Contributor Author

Wow, it just worked. I guess #[cfg] is smarter than we both thought.

Comment thread build.rs
println!("cargo:rustc-cfg=__unicase__default_hasher");
}

if rustc::is_min_version("1.31.0").map(|(is_min, _)| is_min).unwrap_or(true) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We should probably use .unwrap_or(false) since if we can't verify the version then we probably don't want to assume these features are supported.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It's been so long, I can't remember. Something that comes to mind is if nightly were to output an odd version string. Either way, I'm not too concern with it.

@seanmonstar seanmonstar merged commit ffbe8d1 into seanmonstar:master Mar 4, 2019
@abonander
Copy link
Copy Markdown
Contributor Author

@seanmonstar are you waiting for other changes before you cut a release, or...?

@seanmonstar
Copy link
Copy Markdown
Owner

I also upgraded the CaseFolding from 11.0.0 to 12.0.0, and have since published v2.3.0.

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.

Const fn constructors

2 participants