Skip to content

Commit f6289f9

Browse files
nyuriknyurik
committed
Migrate Cargo's &Option<T> into Option<&T>
As part of this, also modify `Option<&String>` to `Option<&str>`, same for `PathBuf` Trying out my new lint rust-lang/rust-clippy#13336 - according to the [video](https://www.youtube.com/watch?v=6c7pZYP_iIE), this could lead to some performance and memory optimizations. Basic thoughts expressed in the video that seem to make sense: * `&Option<T>` in an API breaks encapsulation: * caller must own T and move it into an Option to call with it * if returned, the owner must store it as Option<T> internally in order to return it * Performance is subject to compiler optimization, but at the basics, `&Option<T>` points to memory that has `presence` flag + value, whereas `Option<&T>` by specification is always optimized to a single pointer.
1 parent 2d368ed commit f6289f9

File tree

33 files changed

+174
-150
lines changed

33 files changed

+174
-150
lines changed

‎Cargo.lock

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎benches/benchsuite/benches/global_cache_tracker.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ fn initialize_context() -> GlobalContext {
4343
false,
4444
false,
4545
false,
46-
&None,
46+
None,
4747
&["gc".to_string()],
4848
&[],
4949
)

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ fn main() {
3636
false,
3737
false,
3838
false,
39-
&None,
39+
None,
4040
&["gc".to_string()],
4141
&[],
4242
)

‎benches/benchsuite/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ impl Fixtures {
188188
false,
189189
false,
190190
false,
191-
&Some(self.target_dir()),
191+
Some(&self.target_dir()),
192192
&[],
193193
&[],
194194
)

‎crates/cargo-util-schemas/src/manifest/mod.rs

Lines changed: 29 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -239,8 +239,11 @@ impl TomlPackage {
239239
}
240240
}
241241

242-
pub fn normalized_edition(&self) -> Result<Option<&String>, UnresolvedError> {
243-
self.edition.as_ref().map(|v| v.normalized()).transpose()
242+
pub fn normalized_edition(&self) -> Result<Option<&str>, UnresolvedError> {
243+
self.edition
244+
.as_ref()
245+
.map(|v| v.normalized().map(String::as_str))
246+
.transpose()
244247
}
245248

246249
pub fn normalized_rust_version(&self) -> Result<Option<&RustVersion>, UnresolvedError> {
@@ -258,7 +261,7 @@ impl TomlPackage {
258261
self.authors.as_ref().map(|v| v.normalized()).transpose()
259262
}
260263

261-
pub fn normalized_build(&self) -> Result<Option<&String>, UnresolvedError> {
264+
pub fn normalized_build(&self) -> Result<Option<&str>, UnresolvedError> {
262265
let readme = self.build.as_ref().ok_or(UnresolvedError)?;
263266
match readme {
264267
StringOrBool::Bool(false) => Ok(None),
@@ -279,30 +282,33 @@ impl TomlPackage {
279282
self.publish.as_ref().map(|v| v.normalized()).transpose()
280283
}
281284

282-
pub fn normalized_description(&self) -> Result<Option<&String>, UnresolvedError> {
285+
pub fn normalized_description(&self) -> Result<Option<&str>, UnresolvedError> {
283286
self.description
284287
.as_ref()
285-
.map(|v| v.normalized())
288+
.map(|v| v.normalized().map(String::as_str))
286289
.transpose()
287290
}
288291

289-
pub fn normalized_homepage(&self) -> Result<Option<&String>, UnresolvedError> {
290-
self.homepage.as_ref().map(|v| v.normalized()).transpose()
292+
pub fn normalized_homepage(&self) -> Result<Option<&str>, UnresolvedError> {
293+
self.homepage
294+
.as_ref()
295+
.map(|v| v.normalized().map(String::as_str))
296+
.transpose()
291297
}
292298

293-
pub fn normalized_documentation(&self) -> Result<Option<&String>, UnresolvedError> {
299+
pub fn normalized_documentation(&self) -> Result<Option<&str>, UnresolvedError> {
294300
self.documentation
295301
.as_ref()
296-
.map(|v| v.normalized())
302+
.map(|v| v.normalized().map(String::as_str))
297303
.transpose()
298304
}
299305

300-
pub fn normalized_readme(&self) -> Result<Option<&String>, UnresolvedError> {
306+
pub fn normalized_readme(&self) -> Result<Option<&str>, UnresolvedError> {
301307
let readme = self.readme.as_ref().ok_or(UnresolvedError)?;
302308
readme.normalized().and_then(|sb| match sb {
303309
StringOrBool::Bool(false) => Ok(None),
304310
StringOrBool::Bool(true) => Err(UnresolvedError),
305-
StringOrBool::String(value) => Ok(Some(value)),
311+
StringOrBool::String(value) => Ok(Some(value.as_str())),
306312
})
307313
}
308314

@@ -314,19 +320,25 @@ impl TomlPackage {
314320
self.categories.as_ref().map(|v| v.normalized()).transpose()
315321
}
316322

317-
pub fn normalized_license(&self) -> Result<Option<&String>, UnresolvedError> {
318-
self.license.as_ref().map(|v| v.normalized()).transpose()
323+
pub fn normalized_license(&self) -> Result<Option<&str>, UnresolvedError> {
324+
self.license
325+
.as_ref()
326+
.map(|v| v.normalized().map(String::as_str))
327+
.transpose()
319328
}
320329

321-
pub fn normalized_license_file(&self) -> Result<Option<&String>, UnresolvedError> {
330+
pub fn normalized_license_file(&self) -> Result<Option<&str>, UnresolvedError> {
322331
self.license_file
323332
.as_ref()
324-
.map(|v| v.normalized())
333+
.map(|v| v.normalized().map(String::as_str))
325334
.transpose()
326335
}
327336

328-
pub fn normalized_repository(&self) -> Result<Option<&String>, UnresolvedError> {
329-
self.repository.as_ref().map(|v| v.normalized()).transpose()
337+
pub fn normalized_repository(&self) -> Result<Option<&str>, UnresolvedError> {
338+
self.repository
339+
.as_ref()
340+
.map(|v| v.normalized().map(String::as_str))
341+
.transpose()
330342
}
331343
}
332344

‎crates/resolver-tests/tests/resolve.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ proptest! {
6969
false,
7070
false,
7171
false,
72-
&None,
72+
None,
7373
&["minimal-versions".to_string()],
7474
&[],
7575
)
@@ -110,7 +110,7 @@ proptest! {
110110
false,
111111
false,
112112
false,
113-
&None,
113+
None,
114114
&["direct-minimal-versions".to_string()],
115115
&[],
116116
)
@@ -436,7 +436,7 @@ fn test_resolving_minimum_version_with_transitive_deps() {
436436
false,
437437
false,
438438
false,
439-
&None,
439+
None,
440440
&["minimal-versions".to_string()],
441441
&[],
442442
)

‎crates/xtask-bump-check/src/xtask.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ fn global_context_configure(gctx: &mut GlobalContext, args: &ArgMatches) -> CliR
9898
frozen,
9999
locked,
100100
offline,
101-
&None,
101+
None,
102102
&unstable_flags,
103103
&config_args,
104104
)?;
@@ -333,7 +333,7 @@ fn changed<'r, 'ws>(
333333
let ws_members = ws
334334
.members()
335335
.filter(|pkg| pkg.name() != root_pkg_name) // Only take care of sub crates here.
336-
.filter(|pkg| pkg.publish() != &Some(vec![])) // filter out `publish = false`
336+
.filter(|pkg| pkg.publish() != Some(&vec![])) // filter out `publish = false`
337337
.map(|pkg| {
338338
// Having relative package root path so that we can compare with
339339
// paths of changed files to determine which package has changed.

‎credential/cargo-credential-1password/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
[package]
22
name = "cargo-credential-1password"
3-
version = "0.4.4"
3+
version = "0.5.0"
44
rust-version.workspace = true
55
edition.workspace = true
66
license.workspace = true

‎credential/cargo-credential-1password/src/main.rs

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ impl OnePasswordKeychain {
114114
Ok(Some(buffer))
115115
}
116116

117-
fn make_cmd(&self, session: &Option<String>, args: &[&str]) -> Command {
117+
fn make_cmd(&self, session: Option<&str>, args: &[&str]) -> Command {
118118
let mut cmd = Command::new("op");
119119
cmd.args(args);
120120
if let Some(account) = &self.account {
@@ -153,7 +153,7 @@ impl OnePasswordKeychain {
153153
Ok(buffer)
154154
}
155155

156-
fn search(&self, session: &Option<String>, index_url: &str) -> Result<Option<String>, Error> {
156+
fn search(&self, session: Option<&str>, index_url: &str) -> Result<Option<String>, Error> {
157157
let cmd = self.make_cmd(
158158
session,
159159
&[
@@ -192,7 +192,7 @@ impl OnePasswordKeychain {
192192

193193
fn modify(
194194
&self,
195-
session: &Option<String>,
195+
session: Option<&str>,
196196
id: &str,
197197
token: Secret<&str>,
198198
_name: Option<&str>,
@@ -207,7 +207,7 @@ impl OnePasswordKeychain {
207207

208208
fn create(
209209
&self,
210-
session: &Option<String>,
210+
session: Option<&str>,
211211
index_url: &str,
212212
token: Secret<&str>,
213213
name: Option<&str>,
@@ -235,7 +235,7 @@ impl OnePasswordKeychain {
235235
Ok(())
236236
}
237237

238-
fn get_token(&self, session: &Option<String>, id: &str) -> Result<Secret<String>, Error> {
238+
fn get_token(&self, session: Option<&str>, id: &str) -> Result<Secret<String>, Error> {
239239
let cmd = self.make_cmd(session, &["item", "get", "--format=json", id]);
240240
let buffer = self.run_cmd(cmd)?;
241241
let item: Login = serde_json::from_str(&buffer)
@@ -250,7 +250,7 @@ impl OnePasswordKeychain {
250250
}
251251
}
252252

253-
fn delete(&self, session: &Option<String>, id: &str) -> Result<(), Error> {
253+
fn delete(&self, session: Option<&str>, id: &str) -> Result<(), Error> {
254254
let cmd = self.make_cmd(session, &["item", "delete", id]);
255255
self.run_cmd(cmd)?;
256256
Ok(())
@@ -270,8 +270,8 @@ impl Credential for OnePasswordCredential {
270270
match action {
271271
Action::Get(_) => {
272272
let session = op.signin()?;
273-
if let Some(id) = op.search(&session, registry.index_url)? {
274-
op.get_token(&session, &id)
273+
if let Some(id) = op.search(session.as_deref(), registry.index_url)? {
274+
op.get_token(session.as_deref(), &id)
275275
.map(|token| CredentialResponse::Get {
276276
token,
277277
cache: CacheControl::Session,
@@ -284,21 +284,26 @@ impl Credential for OnePasswordCredential {
284284
Action::Login(options) => {
285285
let session = op.signin()?;
286286
// Check if an item already exists.
287-
if let Some(id) = op.search(&session, registry.index_url)? {
287+
if let Some(id) = op.search(session.as_deref(), registry.index_url)? {
288288
eprintln!("note: token already exists for `{}`", registry.index_url);
289289
let token = cargo_credential::read_token(options, registry)?;
290-
op.modify(&session, &id, token.as_deref(), None)?;
290+
op.modify(session.as_deref(), &id, token.as_deref(), None)?;
291291
} else {
292292
let token = cargo_credential::read_token(options, registry)?;
293-
op.create(&session, registry.index_url, token.as_deref(), None)?;
293+
op.create(
294+
session.as_deref(),
295+
registry.index_url,
296+
token.as_deref(),
297+
None,
298+
)?;
294299
}
295300
Ok(CredentialResponse::Login)
296301
}
297302
Action::Logout => {
298303
let session = op.signin()?;
299304
// Check if an item already exists.
300-
if let Some(id) = op.search(&session, registry.index_url)? {
301-
op.delete(&session, &id)?;
305+
if let Some(id) = op.search(session.as_deref(), registry.index_url)? {
306+
op.delete(session.as_deref(), &id)?;
302307
Ok(CredentialResponse::Logout)
303308
} else {
304309
Err(Error::NotFound)

‎src/bin/cargo/cli.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -436,7 +436,7 @@ fn configure_gctx(
436436
frozen,
437437
locked,
438438
offline,
439-
arg_target_dir,
439+
arg_target_dir.as_deref(),
440440
&unstable_flags,
441441
&config_args,
442442
)?;

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -64,10 +64,10 @@ impl Invocation {
6464
}
6565
}
6666

67-
pub fn add_output(&mut self, path: &Path, link: &Option<PathBuf>) {
67+
pub fn add_output(&mut self, path: &Path, link: Option<&Path>) {
6868
self.outputs.push(path.to_path_buf());
69-
if let Some(ref link) = *link {
70-
self.links.insert(link.clone(), path.to_path_buf());
69+
if let Some(link) = link {
70+
self.links.insert(link.to_path_buf(), path.to_path_buf());
7171
}
7272
}
7373

@@ -134,7 +134,7 @@ impl BuildPlan {
134134

135135
invocation.update_cmd(cmd)?;
136136
for output in outputs.iter() {
137-
invocation.add_output(&output.path, &output.hardlink);
137+
invocation.add_output(&output.path, output.hardlink.as_deref());
138138
}
139139

140140
Ok(())

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

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -555,7 +555,7 @@ fn build_work(build_runner: &mut BuildRunner<'_, '_>, unit: &Unit) -> CargoResul
555555
&script_out_dir,
556556
nightly_features_allowed,
557557
&targets,
558-
&msrv,
558+
msrv.as_ref(),
559559
)?;
560560

561561
if json_messages {
@@ -583,7 +583,7 @@ fn build_work(build_runner: &mut BuildRunner<'_, '_>, unit: &Unit) -> CargoResul
583583
&script_out_dir,
584584
nightly_features_allowed,
585585
&targets_fresh,
586-
&msrv_fresh,
586+
msrv_fresh.as_ref(),
587587
)?,
588588
};
589589

@@ -639,7 +639,7 @@ impl BuildOutput {
639639
script_out_dir: &Path,
640640
nightly_features_allowed: bool,
641641
targets: &[Target],
642-
msrv: &Option<RustVersion>,
642+
msrv: Option<&RustVersion>,
643643
) -> CargoResult<BuildOutput> {
644644
let contents = paths::read_bytes(path)?;
645645
BuildOutput::parse(
@@ -667,7 +667,7 @@ impl BuildOutput {
667667
script_out_dir: &Path,
668668
nightly_features_allowed: bool,
669669
targets: &[Target],
670-
msrv: &Option<RustVersion>,
670+
msrv: Option<&RustVersion>,
671671
) -> CargoResult<BuildOutput> {
672672
let mut library_paths = Vec::new();
673673
let mut library_links = Vec::new();
@@ -717,7 +717,7 @@ impl BuildOutput {
717717

718718
fn check_minimum_supported_rust_version_for_new_syntax(
719719
pkg_descr: &str,
720-
msrv: &Option<RustVersion>,
720+
msrv: Option<&RustVersion>,
721721
flag: &str,
722722
) -> CargoResult<()> {
723723
if let Some(msrv) = msrv {
@@ -1250,7 +1250,7 @@ fn prev_build_output(
12501250
&script_out_dir,
12511251
build_runner.bcx.gctx.nightly_features_allowed,
12521252
unit.pkg.targets(),
1253-
&unit.pkg.rust_version().cloned(),
1253+
unit.pkg.rust_version(),
12541254
)
12551255
.ok(),
12561256
prev_script_out_dir,

0 commit comments

Comments
 (0)