Skip to content

Teach concat!() to concatenate byte str literals #52838

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

Closed
wants to merge 3,623 commits into from

Conversation

estebank
Copy link
Contributor

Fix #52710.

Centril and others added 30 commits February 15, 2018 06:28
rfc, const-repeat-expr: notes on is_rvalue_promotable(expr)
Add missing Copy bound for K in get_default in RFC 2094
Centril and others added 9 commits July 2, 2018 20:32
Allow `loop` in constant evaluation
Update RFC 0430 to allow underscores between numbers in CamelCase names
The Cretonne project is now renamed to Cranelift. Update the names in
the SIMD RFC and add links to the project page.
…unds

RFC: Associated type bounds of form `MyTrait<AssociatedType: Bounds>`
@rust-highfive
Copy link
Contributor

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 29, 2018
@estebank
Copy link
Contributor Author

CC @rust-lang/lang

@kennytm
Copy link
Member

kennytm commented Jul 30, 2018

I think the new behavior of concat! should be placed behind a feature gate.

@rust-highfive

This comment has been minimized.

@@ -1300,6 +1300,11 @@ impl LitKind {
}
}

/// Returns a `LitKind::ByteStr` from `content`.
Copy link
Member

Choose a reason for hiding this comment

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

Why does this need to have a constructor method, specifically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

if b_accumulator.len() > 0 {
base::MacEager::expr(cx.expr_lit(
sp,
ast::LitKind::new_byte_str(b_accumulator),
Copy link
Member

Choose a reason for hiding this comment

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

You might want instead a cx.expr_byte_str.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@estebank
Copy link
Contributor Author

estebank commented Jul 31, 2018

I think the new behavior of concat! should be placed behind a feature gate.

@kennytm other than bad style of introducing insta-stable behavior, do you foresee any problems with the new behavior? If the PR also included support to mix and match "" and b"" (which can be done, we just need to figure out what the appropriate default behavior should be and probably add a lint about it being supported but discouraged), I would definitely agree, but as it is I feel the change is small enough not to warrant a long release cycle.

@joshtriplett
Copy link
Member

@estebank The usual concern there would be if there's some subtle aspect of the new behavior that we don't catch before shipping it, people start relying on that aspect, and we then can't change it even if we want to.

.map(|pos| (pos.shrink_to_lo(), "b".to_string()))
.collect(),
);
err.emit();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just noticed that this error (and suggestion) will be wrong when doing concat!(b"asdf", true, 3).

@rust-highfive

This comment has been minimized.

@estebank
Copy link
Contributor Author

estebank commented Aug 1, 2018

Oh boy... I really messed this up, trying to push to the RFCS repo. Refiled at #52932.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.