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

Rename SliceConcatExt::connect to join #26957

Merged
merged 4 commits into from
Jul 12, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/compiletest/runtest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ fn run_debuginfo_gdb_test(config: &Config, props: &TestProps, testfile: &Path) {
check_lines,
breakpoint_lines
} = parse_debugger_commands(testfile, "gdb");
let mut cmds = commands.connect("\n");
let mut cmds = commands.join("\n");

// compile test file (it should have 'compile-flags:-g' in the header)
let compiler_run_result = compile_test(config, props, testfile);
Expand Down Expand Up @@ -799,7 +799,7 @@ fn cleanup_debug_info_options(options: &Option<String>) -> Option<String> {
split_maybe_args(options).into_iter()
.filter(|x| !options_to_remove.contains(x))
.collect::<Vec<String>>()
.connect(" ");
.join(" ");
Some(new_options)
}

Expand Down Expand Up @@ -1412,15 +1412,15 @@ fn make_cmdline(libpath: &str, prog: &str, args: &[String]) -> String {

// Linux and mac don't require adjusting the library search path
if cfg!(unix) {
format!("{} {}", prog, args.connect(" "))
format!("{} {}", prog, args.join(" "))
} else {
// Build the LD_LIBRARY_PATH variable as it would be seen on the command line
// for diagnostic purposes
fn lib_path_cmd_prefix(path: &str) -> String {
format!("{}=\"{}\"", util::lib_path_env_var(), util::make_new_path(path))
}

format!("{} {} {}", lib_path_cmd_prefix(libpath), prog, args.connect(" "))
format!("{} {} {}", lib_path_cmd_prefix(libpath), prog, args.join(" "))
}
}

Expand Down
18 changes: 17 additions & 1 deletion src/libcollections/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1025,6 +1025,17 @@ pub trait SliceConcatExt<T: ?Sized> {
#[stable(feature = "rust1", since = "1.0.0")]
fn concat(&self) -> Self::Output;

/// Flattens a slice of `T` into a single value `Self::Output`, placing a
/// given separator between each.
///
/// # Examples
///
/// ```
/// assert_eq!(["hello", "world"].join(" "), "hello world");
/// ```
#[stable(feature = "rename_connect_to_join", since = "1.3.0")]
fn join(&self, sep: &T) -> Self::Output;

/// Flattens a slice of `T` into a single value `Self::Output`, placing a
/// given separator between each.
///
Expand All @@ -1034,6 +1045,7 @@ pub trait SliceConcatExt<T: ?Sized> {
/// assert_eq!(["hello", "world"].connect(" "), "hello world");
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
#[deprecated(since = "1.3.0", reason = "renamed to join")]
fn connect(&self, sep: &T) -> Self::Output;
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably best to just have this default to calling join so you don't need multiple impls.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't it backwards-incompatible to do anything other than having join default to calling connect?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, he even had to fix libcollections/str.rs, so it's clearly not backward compatible.

Copy link
Member Author

Choose a reason for hiding this comment

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

The trait is marked unstable but the individual functions are marked as stable. What does that mean in terms of backward-compatibility? I tried to make my changes backwards compatible for users of the trait but I hadn't considered implementors.

Copy link
Contributor

Choose a reason for hiding this comment

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

@eddyb All implementors must have given an impl of connect, so this default will never apply unless you update your impl to not use connect.

Copy link
Member

Choose a reason for hiding this comment

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

Without a default on join, no user impls will compile after this change. But if the trait is unstable... It's probably fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems to me that providing a default on one of the functions is a good idea. I'm just not sure which one.

If we:

  • default connect to call join: then this will break any existing user implementations. However the trait is currently marked unstable. Is breaking backwards compatibility ok then?
  • default join to call connect: then existing user implementations will continue working. However any future users will have to implement a deprecated method which doesn't seem obvious or desirable. Also, will that generate warnings on their implementations?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes. Sorry I saw that this PR was breaking all implementations anyway,
so I immediately paged out "having to implement join" as a breaking change.
:)

Longterm I think connect-defaults-to-join is the right one. Regardless,
sliceconcatext isn't really something that's meant to be impl'd... it's
just a hack we use internally, I think.

On Sat, Jul 11, 2015 at 10:45 AM, Eduard Burtescu notifications@github.com
wrote:

In src/libcollections/slice.rs
#26957 (comment):

 /// assert_eq!(["hello", "world"].connect(" "), "hello world");
 /// ```
 #[stable(feature = "rust1", since = "1.0.0")]
  • #[deprecated(since = "1.3.0", reason = "renamed to join")]
    fn connect(&self, sep: &T) -> Self::Output;

Without a default on join, no user impls will compile after this change.
But if the trait is unstable... It's probably fine.


Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/26957/files#r34414816.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. I'll change connect to default to join which should address the remaining nits. I'll push that change up in a minute. Should I squash everything together or leave the rename and connect-to-join changes in two separate commits?

Copy link
Contributor

Choose a reason for hiding this comment

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

Having a seperate "rename" and "fallout" commit seems reasonable.

On Sat, Jul 11, 2015 at 1:42 PM, Wesley Wiser notifications@github.com
wrote:

In src/libcollections/slice.rs
#26957 (comment):

 /// assert_eq!(["hello", "world"].connect(" "), "hello world");
 /// ```
 #[stable(feature = "rust1", since = "1.0.0")]
  • #[deprecated(since = "1.3.0", reason = "renamed to join")]
    fn connect(&self, sep: &T) -> Self::Output;

Ok. I'll change connect to default to join which should address the
remaining nits. I'll push that change up in a minute. Should I squash
everything together or leave the rename and connect-to-join changes in
two separate commits?


Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/26957/files#r34416058.

}

Expand All @@ -1049,7 +1061,7 @@ impl<T: Clone, V: Borrow<[T]>> SliceConcatExt<T> for [V] {
result
}

fn connect(&self, sep: &T) -> Vec<T> {
fn join(&self, sep: &T) -> Vec<T> {
let size = self.iter().fold(0, |acc, v| acc + v.borrow().len());
let mut result = Vec::with_capacity(size + self.len());
let mut first = true;
Expand All @@ -1059,6 +1071,10 @@ impl<T: Clone, V: Borrow<[T]>> SliceConcatExt<T> for [V] {
}
result
}

fn connect(&self, sep: &T) -> Vec<T> {
self.join(sep)
}
}

/// An iterator that yields the element swaps needed to produce
Expand Down
6 changes: 5 additions & 1 deletion src/libcollections/str.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ impl<S: Borrow<str>> SliceConcatExt<str> for [S] {
result
}

fn connect(&self, sep: &str) -> String {
fn join(&self, sep: &str) -> String {
if self.is_empty() {
return String::new();
}
Expand All @@ -132,6 +132,10 @@ impl<S: Borrow<str>> SliceConcatExt<str> for [S] {
}
result
}

fn connect(&self, sep: &str) -> String {
self.join(sep)
}
}

/*
Expand Down
20 changes: 10 additions & 10 deletions src/libcollectionstest/slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -606,22 +606,22 @@ fn test_concat() {
assert_eq!(d, [1, 2, 3]);

let v: &[&[_]] = &[&[1], &[2, 3]];
assert_eq!(v.connect(&0), [1, 0, 2, 3]);
assert_eq!(v.join(&0), [1, 0, 2, 3]);
let v: &[&[_]] = &[&[1], &[2], &[3]];
assert_eq!(v.connect(&0), [1, 0, 2, 0, 3]);
assert_eq!(v.join(&0), [1, 0, 2, 0, 3]);
}

#[test]
fn test_connect() {
fn test_join() {
let v: [Vec<i32>; 0] = [];
assert_eq!(v.connect(&0), []);
assert_eq!([vec![1], vec![2, 3]].connect(&0), [1, 0, 2, 3]);
assert_eq!([vec![1], vec![2], vec![3]].connect(&0), [1, 0, 2, 0, 3]);
assert_eq!(v.join(&0), []);
assert_eq!([vec![1], vec![2, 3]].join(&0), [1, 0, 2, 3]);
assert_eq!([vec![1], vec![2], vec![3]].join(&0), [1, 0, 2, 0, 3]);

let v: [&[_]; 2] = [&[1], &[2, 3]];
assert_eq!(v.connect(&0), [1, 0, 2, 3]);
assert_eq!(v.join(&0), [1, 0, 2, 3]);
let v: [&[_]; 3] = [&[1], &[2], &[3]];
assert_eq!(v.connect(&0), [1, 0, 2, 0, 3]);
assert_eq!(v.join(&0), [1, 0, 2, 0, 3]);
}

#[test]
Expand Down Expand Up @@ -1339,11 +1339,11 @@ mod bench {
}

#[bench]
fn connect(b: &mut Bencher) {
fn join(b: &mut Bencher) {
let xss: Vec<Vec<i32>> =
(0..100).map(|i| (0..i).collect()).collect();
b.iter(|| {
xss.connect(&0)
xss.join(&0)
});
}

Expand Down
30 changes: 15 additions & 15 deletions src/libcollectionstest/str.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,32 +158,32 @@ fn test_concat_for_different_lengths() {
test_concat!("abc", ["", "a", "bc"]);
}

macro_rules! test_connect {
macro_rules! test_join {
($expected: expr, $string: expr, $delim: expr) => {
{
let s = $string.connect($delim);
let s = $string.join($delim);
assert_eq!($expected, s);
}
}
}

#[test]
fn test_connect_for_different_types() {
test_connect!("a-b", ["a", "b"], "-");
fn test_join_for_different_types() {
test_join!("a-b", ["a", "b"], "-");
let hyphen = "-".to_string();
test_connect!("a-b", [s("a"), s("b")], &*hyphen);
test_connect!("a-b", vec!["a", "b"], &*hyphen);
test_connect!("a-b", &*vec!["a", "b"], "-");
test_connect!("a-b", vec![s("a"), s("b")], "-");
test_join!("a-b", [s("a"), s("b")], &*hyphen);
test_join!("a-b", vec!["a", "b"], &*hyphen);
test_join!("a-b", &*vec!["a", "b"], "-");
test_join!("a-b", vec![s("a"), s("b")], "-");
}

#[test]
fn test_connect_for_different_lengths() {
fn test_join_for_different_lengths() {
let empty: &[&str] = &[];
test_connect!("", empty, "-");
test_connect!("a", ["a"], "-");
test_connect!("a-b", ["a", "b"], "-");
test_connect!("-a-bc", ["", "a", "bc"], "-");
test_join!("", empty, "-");
test_join!("a", ["a"], "-");
test_join!("a-b", ["a", "b"], "-");
test_join!("-a-bc", ["", "a", "bc"], "-");
}

#[test]
Expand Down Expand Up @@ -2081,12 +2081,12 @@ mod bench {
}

#[bench]
fn bench_connect(b: &mut Bencher) {
fn bench_join(b: &mut Bencher) {
let s = "ศไทย中华Việt Nam; Mary had a little lamb, Little lamb";
let sep = "→";
let v = vec![s, s, s, s, s, s, s, s, s, s];
b.iter(|| {
assert_eq!(v.connect(sep).len(), s.len() * 10 + sep.len() * 9);
assert_eq!(v.join(sep).len(), s.len() * 10 + sep.len() * 9);
})
}

Expand Down
6 changes: 3 additions & 3 deletions src/libgetopts/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -784,13 +784,13 @@ pub fn usage(brief: &str, opts: &[OptGroup]) -> String {

// FIXME: #5516 should be graphemes not codepoints
// wrapped description
row.push_str(&desc_rows.connect(&desc_sep[..]));
row.push_str(&desc_rows.join(&desc_sep[..]));

row
});

format!("{}\n\nOptions:\n{}\n", brief,
rows.collect::<Vec<String>>().connect("\n"))
rows.collect::<Vec<String>>().join("\n"))
}

fn format_option(opt: &OptGroup) -> String {
Expand Down Expand Up @@ -836,7 +836,7 @@ pub fn short_usage(program_name: &str, opts: &[OptGroup]) -> String {
line.push_str(&opts.iter()
.map(format_option)
.collect::<Vec<String>>()
.connect(" ")[..]);
.join(" ")[..]);
line
}

Expand Down
2 changes: 1 addition & 1 deletion src/librustc/metadata/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2028,7 +2028,7 @@ fn encode_dylib_dependency_formats(rbml_w: &mut Encoder, ecx: &EncodeContext) {
cstore::RequireStatic => "s",
})).to_string())
}).collect::<Vec<String>>();
rbml_w.wr_tagged_str(tag, &s.connect(","));
rbml_w.wr_tagged_str(tag, &s.join(","));
}
None => {
rbml_w.wr_tagged_str(tag, "");
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1098,7 +1098,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {

pub fn tys_to_string(&self, ts: &[Ty<'tcx>]) -> String {
let tstrs: Vec<String> = ts.iter().map(|t| self.ty_to_string(*t)).collect();
format!("({})", tstrs.connect(", "))
format!("({})", tstrs.join(", "))
}

pub fn trait_ref_to_string(&self, t: &ty::TraitRef<'tcx>) -> String {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_driver/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -843,7 +843,7 @@ fn write_out_deps(sess: &Session,
let mut file = try!(fs::File::create(&deps_filename));
for path in &out_filenames {
try!(write!(&mut file,
"{}: {}\n\n", path.display(), files.connect(" ")));
"{}: {}\n\n", path.display(), files.join(" ")));
}
Ok(())
})();
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_driver/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,7 @@ Available lint options:
for (name, to) in lints {
let name = name.to_lowercase().replace("_", "-");
let desc = to.into_iter().map(|x| x.as_str().replace("_", "-"))
.collect::<Vec<String>>().connect(", ");
.collect::<Vec<String>>().join(", ");
println!(" {} {}",
padded(&name[..]), desc);
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_driver/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ impl UserIdentifiedItem {
fn reconstructed_input(&self) -> String {
match *self {
ItemViaNode(node_id) => node_id.to_string(),
ItemViaPath(ref parts) => parts.connect("::"),
ItemViaPath(ref parts) => parts.join("::"),
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/librustc_driver/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ impl<'a, 'tcx> Env<'a, 'tcx> {
return match search_mod(self, &self.infcx.tcx.map.krate().module, 0, names) {
Some(id) => id,
None => {
panic!("no item found: `{}`", names.connect("::"));
panic!("no item found: `{}`", names.join("::"));
}
};

Expand Down
2 changes: 1 addition & 1 deletion src/librustc_lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -923,7 +923,7 @@ impl NonSnakeCase {
}
words.push(buf);
}
words.connect("_")
words.join("_")
}

fn check_snake_case(&self, cx: &Context, sort: &str, name: &str, span: Option<Span>) {
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_trans/trans/_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -936,7 +936,7 @@ fn compile_guard<'a, 'p, 'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
bcx.to_str(),
guard_expr,
m,
vals.iter().map(|v| bcx.val_to_string(*v)).collect::<Vec<_>>().connect(", "));
vals.iter().map(|v| bcx.val_to_string(*v)).collect::<Vec<_>>().join(", "));
let _indenter = indenter();

let mut bcx = insert_lllocals(bcx, &data.bindings_map, None);
Expand Down Expand Up @@ -981,7 +981,7 @@ fn compile_submatch<'a, 'p, 'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
debug!("compile_submatch(bcx={}, m={:?}, vals=[{}])",
bcx.to_str(),
m,
vals.iter().map(|v| bcx.val_to_string(*v)).collect::<Vec<_>>().connect(", "));
vals.iter().map(|v| bcx.val_to_string(*v)).collect::<Vec<_>>().join(", "));
let _indenter = indenter();
let _icx = push_ctxt("match::compile_submatch");
let mut bcx = bcx;
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_trans/trans/asm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ pub fn trans_inline_asm<'blk, 'tcx>(bcx: Block<'blk, 'tcx>, ia: &ast::InlineAsm)
.chain(arch_clobbers.iter()
.map(|s| s.to_string()))
.collect::<Vec<String>>()
.connect(",");
.join(",");

debug!("Asm Constraints: {}", &all_constraints[..]);

Expand Down
2 changes: 1 addition & 1 deletion src/librustc_trans/trans/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ pub fn Invoke(cx: Block,
terminate(cx, "Invoke");
debug!("Invoke({} with arguments ({}))",
cx.val_to_string(fn_),
args.iter().map(|a| cx.val_to_string(*a)).collect::<Vec<String>>().connect(", "));
args.iter().map(|a| cx.val_to_string(*a)).collect::<Vec<String>>().join(", "));
debug_loc.apply(cx.fcx);
B(cx).invoke(fn_, args, then, catch, attributes)
}
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_trans/trans/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
args.iter()
.map(|&v| self.ccx.tn().val_to_string(v))
.collect::<Vec<String>>()
.connect(", "));
.join(", "));

unsafe {
let v = llvm::LLVMBuildInvoke(self.llbuilder,
Expand Down Expand Up @@ -809,7 +809,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
args.iter()
.map(|&v| self.ccx.tn().val_to_string(v))
.collect::<Vec<String>>()
.connect(", "));
.join(", "));

unsafe {
let v = llvm::LLVMBuildCall(self.llbuilder, llfn, args.as_ptr(),
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_trans/trans/debuginfo/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1443,7 +1443,7 @@ impl<'tcx> EnumMemberDescriptionFactory<'tcx> {
let discrfield = discrfield.iter()
.skip(1)
.map(|x| x.to_string())
.collect::<Vec<_>>().connect("$");
.collect::<Vec<_>>().join("$");
let union_member_name = format!("RUST$ENCODED$ENUM${}${}",
discrfield,
null_variant_name);
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_trans/trans/llrepr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ pub trait LlvmRepr {
impl<T:LlvmRepr> LlvmRepr for [T] {
fn llrepr(&self, ccx: &CrateContext) -> String {
let reprs: Vec<String> = self.iter().map(|t| t.llrepr(ccx)).collect();
format!("[{}]", reprs.connect(","))
format!("[{}]", reprs.join(","))
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/librustc_trans/trans/type_.rs
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ impl TypeNames {

pub fn types_to_str(&self, tys: &[Type]) -> String {
let strs: Vec<String> = tys.iter().map(|t| self.type_to_string(*t)).collect();
format!("[{}]", strs.connect(","))
format!("[{}]", strs.join(","))
}

pub fn val_to_string(&self, val: ValueRef) -> String {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_trans/trans/type_of.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,7 @@ fn llvm_type_name<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>,
let tstr = if strings.is_empty() {
base
} else {
format!("{}<{}>", base, strings.connect(", "))
format!("{}<{}>", base, strings.join(", "))
};

if did.krate == 0 {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_typeck/check/method/suggest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ pub fn report_error<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
p.self_ty(),
p))
.collect::<Vec<_>>()
.connect(", ");
.join(", ");
cx.sess.fileline_note(
span,
&format!("the method `{}` exists but the \
Expand Down
Loading