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

Dogfood str_split_once() #79809

Merged
merged 8 commits into from
Dec 11, 2020
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
1 change: 1 addition & 0 deletions compiler/rustc_mir/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ Rust MIR: a lowered representation of Rust.
#![feature(or_patterns)]
#![feature(once_cell)]
#![feature(control_flow_enum)]
#![feature(str_split_once)]
#![recursion_limit = "256"]

#[macro_use]
Expand Down
62 changes: 34 additions & 28 deletions compiler/rustc_mir/src/transform/coverage/debug.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,40 +148,46 @@ impl DebugOptions {

if let Ok(env_debug_options) = std::env::var(RUSTC_COVERAGE_DEBUG_OPTIONS) {
for setting_str in env_debug_options.replace(" ", "").replace("-", "_").split(',') {
let mut setting = setting_str.splitn(2, '=');
match setting.next() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original match was considering more than necessary. setting.next() was always Some(). Only the value could be None.

I kept value modeled as an Option to avoid needing to change the related code like bool_option_val.

Some(option) if option == "allow_unused_expressions" => {
allow_unused_expressions = bool_option_val(option, setting.next());
let (option, value) = match setting_str.split_once('=') {
None => (setting_str, None),
Some((k, v)) => (k, Some(v)),
};
match option {
"allow_unused_expressions" => {
allow_unused_expressions = bool_option_val(option, value);
debug!(
"{} env option `allow_unused_expressions` is set to {}",
RUSTC_COVERAGE_DEBUG_OPTIONS, allow_unused_expressions
);
}
Some(option) if option == "counter_format" => {
if let Some(strval) = setting.next() {
counter_format = counter_format_option_val(strval);
debug!(
"{} env option `counter_format` is set to {:?}",
RUSTC_COVERAGE_DEBUG_OPTIONS, counter_format
);
} else {
bug!(
"`{}` option in environment variable {} requires one or more \
plus-separated choices (a non-empty subset of \
`id+block+operation`)",
option,
RUSTC_COVERAGE_DEBUG_OPTIONS
);
}
"counter_format" => {
match value {
None => {
bug!(
"`{}` option in environment variable {} requires one or more \
plus-separated choices (a non-empty subset of \
`id+block+operation`)",
option,
RUSTC_COVERAGE_DEBUG_OPTIONS
);
}
Some(val) => {
counter_format = counter_format_option_val(val);
debug!(
"{} env option `counter_format` is set to {:?}",
RUSTC_COVERAGE_DEBUG_OPTIONS, counter_format
);
}
};
}
Some("") => {}
Some(invalid) => bug!(
"Unsupported setting `{}` in environment variable {}",
invalid,
RUSTC_COVERAGE_DEBUG_OPTIONS
),
None => {}
}
_ => {
bug!(
"Unsupported setting `{}` in environment variable {}",
option,
RUSTC_COVERAGE_DEBUG_OPTIONS
)
}
};
}
}

Expand Down
109 changes: 50 additions & 59 deletions compiler/rustc_session/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1296,8 +1296,10 @@ fn parse_output_types(
if !debugging_opts.parse_only {
for list in matches.opt_strs("emit") {
for output_type in list.split(',') {
let mut parts = output_type.splitn(2, '=');
let shorthand = parts.next().unwrap();
let (shorthand, path) = match output_type.split_once('=') {
None => (output_type, None),
Some((shorthand, path)) => (shorthand, Some(PathBuf::from(path))),
};
let output_type = OutputType::from_shorthand(shorthand).unwrap_or_else(|| {
early_error(
error_format,
Expand All @@ -1308,7 +1310,6 @@ fn parse_output_types(
),
)
});
let path = parts.next().map(PathBuf::from);
output_types.insert(output_type, path);
}
}
Expand Down Expand Up @@ -1452,11 +1453,10 @@ fn parse_opt_level(
let max_c = matches
.opt_strs_pos("C")
.into_iter()
.flat_map(
|(i, s)| {
if let Some("opt-level") = s.splitn(2, '=').next() { Some(i) } else { None }
},
)
.flat_map(|(i, s)| {
// NB: This can match a string without `=`.
if let Some("opt-level") = s.splitn(2, '=').next() { Some(i) } else { None }
})
.max();
if max_o > max_c {
OptLevel::Default
Expand Down Expand Up @@ -1491,11 +1491,10 @@ fn select_debuginfo(
let max_c = matches
.opt_strs_pos("C")
.into_iter()
.flat_map(
|(i, s)| {
if let Some("debuginfo") = s.splitn(2, '=').next() { Some(i) } else { None }
},
)
.flat_map(|(i, s)| {
// NB: This can match a string without `=`.
if let Some("debuginfo") = s.splitn(2, '=').next() { Some(i) } else { None }
})
.max();
if max_g > max_c {
DebugInfo::Full
Expand Down Expand Up @@ -1528,23 +1527,26 @@ fn parse_libs(
.map(|s| {
// Parse string of the form "[KIND=]lib[:new_name]",
// where KIND is one of "dylib", "framework", "static".
let mut parts = s.splitn(2, '=');
let kind = parts.next().unwrap();
let (name, kind) = match (parts.next(), kind) {
(None, name) => (name, NativeLibKind::Unspecified),
(Some(name), "dylib") => (name, NativeLibKind::Dylib),
(Some(name), "framework") => (name, NativeLibKind::Framework),
(Some(name), "static") => (name, NativeLibKind::StaticBundle),
(Some(name), "static-nobundle") => (name, NativeLibKind::StaticNoBundle),
(_, s) => {
early_error(
error_format,
&format!(
"unknown library kind `{}`, expected \
one of dylib, framework, or static",
s
),
);
Copy link
Member

@matklad matklad Dec 8, 2020

Choose a reason for hiding this comment

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

Just writing my reasoning down for posterity:

The previous version is more compact, so I am almost inclined to say that it was better. However, the new version doesn't have .unwrap(), and, given a couple of instances where have impossible cases in other parts of this PR, I think a tad more verbosity is a fine price to pay for making illegal states unrepresentable here.

That said, would flattening the two matches into one work better here?

match s.split_once('=') {
  Some(("dylib", name)) => ()
}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack that this is less compact, but I refactored to use the nested match because it reduces duplication and imo makes this code easier to grok, albeit longer. We no longer have to put name in multiple arms of the match, like we had previously:

match s.split_once('=') {
  Some(("dylib", name)) => (),
  Some(("framework", name)) => (),
  ...
}

Instead, what do you think about adding impl TryFrom<String> for NativeLibKind? I'm happy to add that (although I might recommend saving that for a followup to keep this PR more focused).

Copy link
Member

Choose a reason for hiding this comment

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

Instead, what do you think about adding impl TryFrom for NativeLibKind?

Not TryFrom, but impl FromStr for NativeLibKind would work nicely. But I think the added benefit here is marginal, if any (there's drawback of more code churn), so I suggest leaving the code as is then!

let (name, kind) = match s.split_once('=') {
None => (s, NativeLibKind::Unspecified),
Some((kind, name)) => {
let kind = match kind {
"dylib" => NativeLibKind::Dylib,
"framework" => NativeLibKind::Framework,
"static" => NativeLibKind::StaticBundle,
"static-nobundle" => NativeLibKind::StaticNoBundle,
s => {
early_error(
error_format,
&format!(
"unknown library kind `{}`, expected \
one of dylib, framework, or static",
s
),
);
}
};
(name.to_string(), kind)
}
};
if kind == NativeLibKind::StaticNoBundle
Expand All @@ -1556,10 +1558,11 @@ fn parse_libs(
accepted on the nightly compiler",
);
}
let mut name_parts = name.splitn(2, ':');
let name = name_parts.next().unwrap();
let new_name = name_parts.next();
(name.to_owned(), new_name.map(|n| n.to_owned()), kind)
let (name, new_name) = match name.split_once(':') {
None => (name, None),
Some((name, new_name)) => (name.to_string(), Some(new_name.to_owned())),
};
(name, new_name, kind)
})
.collect()
}
Expand All @@ -1580,20 +1583,13 @@ pub fn parse_externs(
let is_unstable_enabled = debugging_opts.unstable_options;
let mut externs: BTreeMap<String, ExternEntry> = BTreeMap::new();
for arg in matches.opt_strs("extern") {
let mut parts = arg.splitn(2, '=');
let name = parts
.next()
.unwrap_or_else(|| early_error(error_format, "--extern value must not be empty"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case could never happen.

Copy link
Member

Choose a reason for hiding this comment

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

Niiiice!

let path = parts.next().map(|s| s.to_string());

let mut name_parts = name.splitn(2, ':');
let first_part = name_parts.next();
let second_part = name_parts.next();
let (options, name) = match (first_part, second_part) {
(Some(opts), Some(name)) => (Some(opts), name),
(Some(name), None) => (None, name),
(None, None) => early_error(error_format, "--extern name must not be empty"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case could never happen, the first value is always Some.

_ => unreachable!(),
let (name, path) = match arg.split_once('=') {
None => (arg, None),
Some((name, path)) => (name.to_string(), Some(path.to_string())),
};
let (options, name) = match name.split_once(':') {
None => (None, name),
Some((opts, name)) => (Some(opts), name.to_string()),
};

let entry = externs.entry(name.to_owned());
Expand Down Expand Up @@ -1682,17 +1678,12 @@ fn parse_remap_path_prefix(
matches
.opt_strs("remap-path-prefix")
.into_iter()
.map(|remap| {
let mut parts = remap.rsplitn(2, '='); // reverse iterator
let to = parts.next();
let from = parts.next();
match (from, to) {
(Some(from), Some(to)) => (PathBuf::from(from), PathBuf::from(to)),
_ => early_error(
error_format,
"--remap-path-prefix must contain '=' between FROM and TO",
),
}
.map(|remap| match remap.rsplit_once('=') {
None => early_error(
error_format,
"--remap-path-prefix must contain '=' between FROM and TO",
),
Some((from, to)) => (PathBuf::from(from), PathBuf::from(to)),
})
.collect()
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_session/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#![feature(crate_visibility_modifier)]
#![feature(once_cell)]
#![feature(or_patterns)]
#![feature(str_split_once)]

#[macro_use]
extern crate bitflags;
Expand Down
7 changes: 4 additions & 3 deletions compiler/rustc_session/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,9 +179,10 @@ macro_rules! options {
{
let mut op = $defaultfn();
for option in matches.opt_strs($prefix) {
let mut iter = option.splitn(2, '=');
let key = iter.next().unwrap();
let value = iter.next();
let (key, value) = match option.split_once('=') {
None => (option, None),
Some((k, v)) => (k.to_string(), Some(v)),
};
let option_to_lookup = key.replace("-", "_");
let mut found = false;
for &(candidate, setter, type_desc, _) in $stat {
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_target/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#![feature(never_type)]
#![feature(associated_type_bounds)]
#![feature(exhaustive_patterns)]
#![feature(str_split_once)]

#[macro_use]
extern crate rustc_macros;
Expand Down
5 changes: 1 addition & 4 deletions compiler/rustc_target/src/spec/apple_base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,7 @@ fn macos_deployment_target() -> (u32, u32) {
let deployment_target = env::var("MACOSX_DEPLOYMENT_TARGET").ok();
let version = deployment_target
.as_ref()
.and_then(|s| {
let mut i = s.splitn(2, '.');
i.next().and_then(|a| i.next().map(|b| (a, b)))
})
.and_then(|s| s.split_once('.'))
.and_then(|(a, b)| a.parse::<u32>().and_then(|a| b.parse::<u32>().map(|b| (a, b))).ok());

version.unwrap_or((10, 7))
Expand Down
1 change: 1 addition & 0 deletions library/std/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,7 @@
#![feature(stdsimd)]
#![feature(stmt_expr_attributes)]
#![feature(str_internals)]
#![feature(str_split_once)]
#![feature(test)]
#![feature(thread_local)]
#![feature(thread_local_internals)]
Expand Down
5 changes: 1 addition & 4 deletions library/std/src/sys_common/net.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,11 +177,8 @@ impl TryFrom<&str> for LookupHost {
}

// split the string by ':' and convert the second part to u16
let mut parts_iter = s.rsplitn(2, ':');
let port_str = try_opt!(parts_iter.next(), "invalid socket address");
let host = try_opt!(parts_iter.next(), "invalid socket address");
let (host, port_str) = try_opt!(s.rsplit_once(':'), "invalid socket address");
let port: u16 = try_opt!(port_str.parse().ok(), "invalid port value");

(host, port).try_into()
}
}
Expand Down
1 change: 1 addition & 0 deletions library/test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#![feature(termination_trait_lib)]
#![feature(test)]
#![feature(total_cmp)]
#![feature(str_split_once)]

// Public reexports
pub use self::bench::{black_box, Bencher};
Expand Down
24 changes: 9 additions & 15 deletions library/test/src/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,30 +105,24 @@ impl TimeThreshold {
/// value.
pub fn from_env_var(env_var_name: &str) -> Option<Self> {
let durations_str = env::var(env_var_name).ok()?;
let (warn_str, critical_str) = durations_str.split_once(',').unwrap_or_else(|| {
panic!(
"Duration variable {} expected to have 2 numbers separated by comma, but got {}",
env_var_name, durations_str
)
});
Comment on lines +108 to +113
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this inverts the order of validation: now we first validate that the split works, and only then validate that they can be parsed as numbers.


// Split string into 2 substrings by comma and try to parse numbers.
let mut durations = durations_str.splitn(2, ',').map(|v| {
let parse_u64 = |v| {
u64::from_str(v).unwrap_or_else(|_| {
panic!(
"Duration value in variable {} is expected to be a number, but got {}",
env_var_name, v
)
})
});

// Callback to be called if the environment variable has unexpected structure.
let panic_on_incorrect_value = || {
panic!(
"Duration variable {} expected to have 2 numbers separated by comma, but got {}",
env_var_name, durations_str
);
};

let (warn, critical) = (
durations.next().unwrap_or_else(panic_on_incorrect_value),
durations.next().unwrap_or_else(panic_on_incorrect_value),
);

let warn = parse_u64(warn_str);
let critical = parse_u64(critical_str);
if warn > critical {
panic!("Test execution warn time should be less or equal to the critical time");
}
Expand Down
15 changes: 5 additions & 10 deletions src/librustdoc/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -397,12 +397,9 @@ impl Options {
matches
.opt_strs("default-setting")
.iter()
.map(|s| {
let mut kv = s.splitn(2, '=');
// never panics because `splitn` always returns at least one element
let k = kv.next().unwrap().to_string();
let v = kv.next().unwrap_or("true").to_string();
(k, v)
.map(|s| match s.split_once('=') {
None => (s.clone(), "true".to_string()),
Some((k, v)) => (k.to_string(), v.to_string()),
})
.collect(),
];
Expand Down Expand Up @@ -707,11 +704,9 @@ fn parse_extern_html_roots(
) -> Result<BTreeMap<String, String>, &'static str> {
let mut externs = BTreeMap::new();
for arg in &matches.opt_strs("extern-html-root-url") {
let mut parts = arg.splitn(2, '=');
let name = parts.next().ok_or("--extern-html-root-url must not be empty")?;
Copy link
Member

Choose a reason for hiding this comment

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

👍

let url = parts.next().ok_or("--extern-html-root-url must be of the form name=url")?;
let (name, url) =
arg.split_once('=').ok_or("--extern-html-root-url must be of the form name=url")?;
externs.insert(name.to_string(), url.to_string());
}

Ok(externs)
}
Loading