Skip to content

Commit f6cea42

Browse files
weihangloweihanglo
authored
refactor: replace InternedString with Cow in IndexPackage (#15559)
### What does this PR try to resolve? ref #14834 As described in the issue, we want to move IndexPackage into cargo-util-schemas. However, it contains InternedString fields, which we don't want to expose as part of the public API. This PR replaces InternedString with Cow. And also, as @weihanglo's suggested, I implemented the From/Into trait to simplify code. From Cow tarit implementation: [`42f593f` (#15559)](42f593f) Replace InternedString with `into()` across the whole codebase: [`5f90098` (#15559)](5f90098) I am unsure if it worsens the readability. Feel free to comment. ### How should we test and review this PR? It shouldn't change or break any tests. Benchmark 1: #15559 (comment) Benchmark 2: #15559 (comment) ### Additional information None
2 parents 24a46c3 + 5f90098 commit f6cea42

File tree

25 files changed

+92
-106
lines changed

25 files changed

+92
-106
lines changed

‎benches/benchsuite/src/bin/capture-last-use.rs

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
1515
use cargo::core::global_cache_tracker::{self, DeferredGlobalLastUse, GlobalCacheTracker};
1616
use cargo::util::cache_lock::CacheLockMode;
17-
use cargo::util::interning::InternedString;
1817
use cargo::GlobalContext;
1918
use rand::prelude::*;
2019
use std::collections::HashMap;
@@ -57,7 +56,7 @@ fn main() {
5756
let cache_dir = real_home.join("registry/cache");
5857
for dir_ent in fs::read_dir(cache_dir).unwrap() {
5958
let registry = dir_ent.unwrap();
60-
let encoded_registry_name = InternedString::new(&registry.file_name().to_string_lossy());
59+
let encoded_registry_name = registry.file_name().to_string_lossy().into();
6160
for krate in fs::read_dir(registry.path()).unwrap() {
6261
let krate = krate.unwrap();
6362
let meta = krate.metadata().unwrap();
@@ -77,7 +76,7 @@ fn main() {
7776
let cache_dir = real_home.join("registry/src");
7877
for dir_ent in fs::read_dir(cache_dir).unwrap() {
7978
let registry = dir_ent.unwrap();
80-
let encoded_registry_name = InternedString::new(&registry.file_name().to_string_lossy());
79+
let encoded_registry_name = registry.file_name().to_string_lossy().into();
8180
for krate in fs::read_dir(registry.path()).unwrap() {
8281
let krate = krate.unwrap();
8382
let meta = krate.metadata().unwrap();
@@ -95,7 +94,7 @@ fn main() {
9594
let git_co_dir = real_home.join("git/checkouts");
9695
for dir_ent in fs::read_dir(git_co_dir).unwrap() {
9796
let git_source = dir_ent.unwrap();
98-
let encoded_git_name = InternedString::new(&git_source.file_name().to_string_lossy());
97+
let encoded_git_name = git_source.file_name().to_string_lossy().into();
9998
for co in fs::read_dir(git_source.path()).unwrap() {
10099
let co = co.unwrap();
101100
let meta = co.metadata().unwrap();

‎src/bin/cargo/commands/add.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ use cargo::ops::cargo_add::AddOptions;
1010
use cargo::ops::cargo_add::DepOp;
1111
use cargo::ops::resolve_ws;
1212
use cargo::util::command_prelude::*;
13-
use cargo::util::interning::InternedString;
1413
use cargo::util::toml_mut::manifest::DepTable;
1514
use cargo::CargoResult;
1615

@@ -287,7 +286,7 @@ fn parse_dependencies(gctx: &GlobalContext, matches: &ArgMatches) -> CargoResult
287286
.map(String::as_str)
288287
.flat_map(parse_feature)
289288
{
290-
let parsed_value = FeatureValue::new(InternedString::new(feature));
289+
let parsed_value = FeatureValue::new(feature.into());
291290
match parsed_value {
292291
FeatureValue::Feature(_) => {
293292
if 1 < crates.len() {

‎src/bin/cargo/commands/rustc.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
use crate::command_prelude::*;
22
use cargo::ops;
3-
use cargo::util::interning::InternedString;
43

54
const PRINT_ARG_NAME: &str = "print";
65
const CRATE_TYPE_ARG_NAME: &str = "crate-type";
@@ -77,7 +76,7 @@ pub fn exec(gctx: &mut GlobalContext, args: &ArgMatches) -> CliResult {
7776
ProfileChecking::LegacyRustc,
7877
)?;
7978
if compile_opts.build_config.requested_profile == "check" {
80-
compile_opts.build_config.requested_profile = InternedString::new("dev");
79+
compile_opts.build_config.requested_profile = "dev".into();
8180
}
8281
let target_args = values(args, "args");
8382
compile_opts.target_rustc_args = if target_args.is_empty() {

‎src/cargo/core/compiler/build_config.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ impl BuildConfig {
116116
requested_kinds,
117117
jobs,
118118
keep_going,
119-
requested_profile: InternedString::new("dev"),
119+
requested_profile: "dev".into(),
120120
intent,
121121
message_format: MessageFormat::Human,
122122
force_rebuild: false,

‎src/cargo/core/compiler/fingerprint/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -694,7 +694,7 @@ impl<'de> Deserialize<'de> for DepFingerprint {
694694
let (pkg_id, name, public, hash) = <(u64, String, bool, u64)>::deserialize(d)?;
695695
Ok(DepFingerprint {
696696
pkg_id,
697-
name: InternedString::new(&name),
697+
name: name.into(),
698698
public,
699699
fingerprint: Arc::new(Fingerprint {
700700
memoized_hash: Mutex::new(Some(hash)),

‎src/cargo/core/dependency.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -632,7 +632,7 @@ impl ArtifactKind {
632632
_ => {
633633
return kind
634634
.strip_prefix("bin:")
635-
.map(|bin_name| ArtifactKind::SelectedBinary(InternedString::new(bin_name)))
635+
.map(|bin_name| ArtifactKind::SelectedBinary(bin_name.into()))
636636
.ok_or_else(|| anyhow::anyhow!("'{}' is not a valid artifact specifier", kind))
637637
}
638638
})

‎src/cargo/core/package.rs

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -208,14 +208,7 @@ impl Package {
208208
let crate_features = summary
209209
.features()
210210
.iter()
211-
.map(|(k, v)| {
212-
(
213-
*k,
214-
v.iter()
215-
.map(|fv| InternedString::new(&fv.to_string()))
216-
.collect(),
217-
)
218-
})
211+
.map(|(k, v)| (*k, v.iter().map(|fv| fv.to_string().into()).collect()))
219212
.collect();
220213

221214
SerializedPackage {
@@ -443,7 +436,7 @@ impl<'gctx> PackageSet<'gctx> {
443436
))),
444437
downloads_finished: 0,
445438
downloaded_bytes: 0,
446-
largest: (0, InternedString::new("")),
439+
largest: (0, "".into()),
447440
success: false,
448441
updated_at: Cell::new(Instant::now()),
449442
timeout,

‎src/cargo/core/package_id.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ impl<'de> de::Deserialize<'de> for PackageId {
7878
let (field, rest) = string
7979
.split_once(' ')
8080
.ok_or_else(|| de::Error::custom("invalid serialized PackageId"))?;
81-
let name = InternedString::new(field);
81+
let name = field.into();
8282

8383
let (field, rest) = rest
8484
.split_once(' ')

‎src/cargo/core/profiles.rs

Lines changed: 24 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ impl Profiles {
9393
// Merge with predefined profiles.
9494
use std::collections::btree_map::Entry;
9595
for (predef_name, mut predef_prof) in Self::predefined_profiles().into_iter() {
96-
match profiles.entry(InternedString::new(predef_name)) {
96+
match profiles.entry(predef_name.into()) {
9797
Entry::Vacant(vac) => {
9898
vac.insert(predef_prof);
9999
}
@@ -119,9 +119,9 @@ impl Profiles {
119119
/// Returns the hard-coded directory names for built-in profiles.
120120
fn predefined_dir_names() -> HashMap<InternedString, InternedString> {
121121
[
122-
(InternedString::new("dev"), InternedString::new("debug")),
123-
(InternedString::new("test"), InternedString::new("debug")),
124-
(InternedString::new("bench"), InternedString::new("release")),
122+
("dev".into(), "debug".into()),
123+
("test".into(), "debug".into()),
124+
("bench".into(), "release".into()),
125125
]
126126
.into()
127127
}
@@ -134,12 +134,12 @@ impl Profiles {
134134
trim_paths_enabled: bool,
135135
) {
136136
profile_makers.by_name.insert(
137-
InternedString::new("dev"),
137+
"dev".into(),
138138
ProfileMaker::new(Profile::default_dev(), profiles.get("dev").cloned()),
139139
);
140140

141141
profile_makers.by_name.insert(
142-
InternedString::new("release"),
142+
"release".into(),
143143
ProfileMaker::new(
144144
Profile::default_release(trim_paths_enabled),
145145
profiles.get("release").cloned(),
@@ -185,7 +185,7 @@ impl Profiles {
185185
match &profile.dir_name {
186186
None => {}
187187
Some(dir_name) => {
188-
self.dir_names.insert(name, InternedString::new(dir_name));
188+
self.dir_names.insert(name, dir_name.into());
189189
}
190190
}
191191

@@ -230,7 +230,7 @@ impl Profiles {
230230
self.get_profile_maker(&inherits_name).unwrap().clone()
231231
}
232232
Some(inherits_name) => {
233-
let inherits_name = InternedString::new(&inherits_name);
233+
let inherits_name = inherits_name.into();
234234
if !set.insert(inherits_name) {
235235
bail!(
236236
"profile inheritance loop detected with profile `{}` inheriting `{}`",
@@ -297,7 +297,7 @@ impl Profiles {
297297
CompileKind::Target(target) => target.short_name(),
298298
};
299299
if target.contains("-apple-") {
300-
profile.split_debuginfo = Some(InternedString::new("unpacked"));
300+
profile.split_debuginfo = Some("unpacked".into());
301301
}
302302
}
303303

@@ -455,7 +455,7 @@ impl ProfileMaker {
455455
// basically turning down the optimization level and avoid limiting
456456
// codegen units. This ensures that we spend little time optimizing as
457457
// well as enabling parallelism by not constraining codegen units.
458-
profile.opt_level = InternedString::new("0");
458+
profile.opt_level = "0".into();
459459
profile.codegen_units = None;
460460

461461
// For build dependencies, we usually don't need debuginfo, and
@@ -531,12 +531,12 @@ fn merge_toml_overrides(
531531
/// Does not merge overrides (see `merge_toml_overrides`).
532532
fn merge_profile(profile: &mut Profile, toml: &TomlProfile) {
533533
if let Some(ref opt_level) = toml.opt_level {
534-
profile.opt_level = InternedString::new(&opt_level.0);
534+
profile.opt_level = opt_level.0.as_str().into();
535535
}
536536
match toml.lto {
537537
Some(StringOrBool::Bool(b)) => profile.lto = Lto::Bool(b),
538538
Some(StringOrBool::String(ref n)) if is_off(n.as_str()) => profile.lto = Lto::Off,
539-
Some(StringOrBool::String(ref n)) => profile.lto = Lto::Named(InternedString::new(n)),
539+
Some(StringOrBool::String(ref n)) => profile.lto = Lto::Named(n.into()),
540540
None => {}
541541
}
542542
if toml.codegen_backend.is_some() {
@@ -552,7 +552,7 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) {
552552
profile.debug_assertions = debug_assertions;
553553
}
554554
if let Some(split_debuginfo) = &toml.split_debuginfo {
555-
profile.split_debuginfo = Some(InternedString::new(split_debuginfo));
555+
profile.split_debuginfo = Some(split_debuginfo.into());
556556
}
557557
if let Some(rpath) = toml.rpath {
558558
profile.rpath = rpath;
@@ -578,16 +578,12 @@ fn merge_profile(profile: &mut Profile, toml: &TomlProfile) {
578578
profile.trim_paths = Some(trim_paths.clone());
579579
}
580580
profile.strip = match toml.strip {
581-
Some(StringOrBool::Bool(true)) => {
582-
Strip::Resolved(StripInner::Named(InternedString::new("symbols")))
583-
}
581+
Some(StringOrBool::Bool(true)) => Strip::Resolved(StripInner::Named("symbols".into())),
584582
Some(StringOrBool::Bool(false)) => Strip::Resolved(StripInner::None),
585583
Some(StringOrBool::String(ref n)) if n.as_str() == "none" => {
586584
Strip::Resolved(StripInner::None)
587585
}
588-
Some(StringOrBool::String(ref n)) => {
589-
Strip::Resolved(StripInner::Named(InternedString::new(n)))
590-
}
586+
Some(StringOrBool::String(ref n)) => Strip::Resolved(StripInner::Named(n.into())),
591587
None => Strip::Deferred(StripInner::None),
592588
};
593589
}
@@ -635,8 +631,8 @@ pub struct Profile {
635631
impl Default for Profile {
636632
fn default() -> Profile {
637633
Profile {
638-
name: InternedString::new(""),
639-
opt_level: InternedString::new("0"),
634+
name: "".into(),
635+
opt_level: "0".into(),
640636
root: ProfileRoot::Debug,
641637
lto: Lto::Bool(false),
642638
codegen_backend: None,
@@ -710,7 +706,7 @@ impl Profile {
710706
/// Returns a built-in `dev` profile.
711707
fn default_dev() -> Profile {
712708
Profile {
713-
name: InternedString::new("dev"),
709+
name: "dev".into(),
714710
root: ProfileRoot::Debug,
715711
debuginfo: DebugInfo::Resolved(TomlDebugInfo::Full),
716712
debug_assertions: true,
@@ -724,9 +720,9 @@ impl Profile {
724720
fn default_release(trim_paths_enabled: bool) -> Profile {
725721
let trim_paths = trim_paths_enabled.then(|| TomlTrimPathsValue::Object.into());
726722
Profile {
727-
name: InternedString::new("release"),
723+
name: "release".into(),
728724
root: ProfileRoot::Release,
729-
opt_level: InternedString::new("3"),
725+
opt_level: "3".into(),
730726
trim_paths,
731727
..Profile::default()
732728
}
@@ -1274,13 +1270,13 @@ fn merge_config_profiles(
12741270
profile.merge(&config_profile);
12751271
}
12761272
if let Some(inherits) = &profile.inherits {
1277-
check_to_add.insert(InternedString::new(inherits));
1273+
check_to_add.insert(inherits.into());
12781274
}
12791275
}
12801276
// Add the built-in profiles. This is important for things like `cargo
12811277
// test` which implicitly use the "dev" profile for dependencies.
1282-
for name in &["dev", "release", "test", "bench"] {
1283-
check_to_add.insert(InternedString::new(name));
1278+
for name in ["dev", "release", "test", "bench"] {
1279+
check_to_add.insert(name.into());
12841280
}
12851281
// Add config-only profiles.
12861282
// Need to iterate repeatedly to get all the inherits values.
@@ -1291,7 +1287,7 @@ fn merge_config_profiles(
12911287
if !profiles.contains_key(name.as_str()) {
12921288
if let Some(config_profile) = get_config_profile(ws, &name)? {
12931289
if let Some(inherits) = &config_profile.inherits {
1294-
check_to_add.insert(InternedString::new(inherits));
1290+
check_to_add.insert(inherits.into());
12951291
}
12961292
profiles.insert(name, config_profile);
12971293
}

‎src/cargo/core/resolver/features.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,7 @@ impl CliFeatures {
306306
.flat_map(|s| s.split_whitespace())
307307
.flat_map(|s| s.split(','))
308308
.filter(|s| !s.is_empty())
309-
.map(InternedString::new)
309+
.map(|s| s.into())
310310
.map(FeatureValue::new)
311311
.collect()
312312
}

‎src/cargo/core/summary.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -377,15 +377,15 @@ impl FeatureValue {
377377
Some((dep, dep_feat)) => {
378378
let dep_name = dep.strip_suffix('?');
379379
FeatureValue::DepFeature {
380-
dep_name: InternedString::new(dep_name.unwrap_or(dep)),
381-
dep_feature: InternedString::new(dep_feat),
380+
dep_name: dep_name.unwrap_or(dep).into(),
381+
dep_feature: dep_feat.into(),
382382
weak: dep_name.is_some(),
383383
}
384384
}
385385
None => {
386386
if let Some(dep_name) = feature.strip_prefix("dep:") {
387387
FeatureValue::Dep {
388-
dep_name: InternedString::new(dep_name),
388+
dep_name: dep_name.into(),
389389
}
390390
} else {
391391
FeatureValue::Feature(feature)

‎src/cargo/core/workspace.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1549,7 +1549,7 @@ impl<'gctx> Workspace<'gctx> {
15491549
.flatten()
15501550
.unique()
15511551
.filter(|element| {
1552-
let feature = FeatureValue::new(InternedString::new(element));
1552+
let feature = FeatureValue::new(element.into());
15531553
!cli_features.features.contains(&feature) && !found_features.contains(&feature)
15541554
})
15551555
.sorted()

‎src/cargo/ops/cargo_output_metadata.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -302,7 +302,7 @@ fn build_resolve_graph_r(
302302
// Given that Cargo doesn't know which target it should resolve to,
303303
// when an artifact dep is specified with { target = "target" },
304304
// keep it with a special "<target>" string,
305-
.or_else(|| Some(InternedString::new("<target>"))),
305+
.or_else(|| Some("<target>".into())),
306306
None => None,
307307
};
308308

@@ -334,7 +334,7 @@ fn build_resolve_graph_r(
334334
},
335335
// No lib target exists but contains artifact deps.
336336
(None, 1..) => Dep {
337-
name: InternedString::new(""),
337+
name: "".into(),
338338
pkg: pkg_id.to_spec(),
339339
pkg_id,
340340
dep_kinds,

‎src/cargo/ops/registry/info/view.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ pub(super) fn pretty_view(
135135
)?;
136136
}
137137

138-
let activated = &[InternedString::new("default")];
138+
let activated = &["default".into()];
139139
let resolved_features = resolve_features(activated, summary.features());
140140
pretty_features(
141141
resolved_features.clone(),

0 commit comments

Comments
 (0)