Skip to content

Commit

Permalink
Make vec_map required dependency - in fact, it already is
Browse files Browse the repository at this point in the history
  • Loading branch information
CreepySkeleton committed Apr 24, 2020
1 parent 7b5e2cf commit 739e704
Show file tree
Hide file tree
Showing 5 changed files with 5 additions and 104 deletions.
4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,11 @@ unicode-width = "0.1"
textwrap = "0.11"
indexmap = "1.0"
os_str_bytes = "2.1"
vec_map = "0.8"

This comment has been minimized.

Copy link
@BurntSushi

BurntSushi May 12, 2020

Contributor

@CreepySkeleton @kbknapp Could you say more about why vec_map was made a required dependency? I've turned it off in ripgrep and haven't missed any extra performance gains that it is supposed to provide. Could we please retain the ability to turn it off?

This comment has been minimized.

Copy link
@kbknapp

kbknapp May 12, 2020

Member

That's my mistake for not following all development closely over the past months. I'm trying to get back into the swing of things. I've added new comments to #1365 about this. @CreepySkeleton can expand on why it was changed, but I'm guessing because vec_map is a small dep, that when included actually made the resulting binary smaller while also providing a small (negligible?) perf bump.

In fact, since you're here, if you could elaborate or add your thoughts to that linked issue it would help from the perspective of reducing dep graphs, etc.

This comment has been minimized.

Copy link
@pksunkara

pksunkara May 12, 2020

Member

binary smaller while also providing a small (negligible?) perf bump

Yup, I had checked it when reviewing the PR. It also reduced the maintenance burden by deleting some piece of code.

This comment has been minimized.

Copy link
@CreepySkeleton

CreepySkeleton May 13, 2020

Author Contributor

In two words, we used to have two implementations of VecMap: the external one that worked like charm and the embedded one that was based on BTreeMap. The situation was: we had two competing implementations of the same thing and one of them was objectively better (code size + no change in compilation time). That's the whole story.

strsim = { version = "0.10", optional = true }
yaml-rust = { version = "0.4.1", optional = true }
atty = { version = "0.2", optional = true }
termcolor = { version = "1.1", optional = true }
vec_map = { version = "0.8", optional = true }
term_size = { version = "1.0.0-beta1", optional = true }
lazy_static = { version = "1", optional = true }
clap_derive = { path = "./clap_derive", version = "3.0.0-beta.1", optional = true }
Expand All @@ -80,7 +80,7 @@ version-sync = "0.8"
criterion = { git = "git://github.com/pksunkara/criterion.rs", version = "0.3" }

[features]
default = ["suggestions", "color", "vec_map", "derive", "std", "cargo"]
default = ["suggestions", "color", "derive", "std", "cargo"]
std = [] # support for no_std in a backwards-compatible way
suggestions = ["strsim"]
color = ["atty", "termcolor"]
Expand Down
1 change: 0 additions & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,6 @@
//! * `derive`: Enables the custom derive (i.e. `#[derive(Clap)]`). Without this you must use one of the other methods of creating a `clap` CLI listed above
//! * `suggestions`: Turns on the `Did you mean '--myoption'?` feature for when users make typos. (builds dependency `strsim`)
//! * `color`: Turns on colored error messages. This feature only works on non-Windows OSs. (builds dependency `ansi-term`)
//! * `vec_map`: Use [`VecMap`](https://crates.io/crates/vec_map) internally instead of a [`BTreeMap`](https://doc.rust-lang.org/stable/std/collections/struct.BTreeMap.html). This feature provides a _slight_ performance improvement. (builds dependency `vec_map`)
//!
//! To disable these, add this to your `Cargo.toml`:
//!
Expand Down
8 changes: 1 addition & 7 deletions src/parse/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,16 +127,10 @@ where
// Because you must wait until all arguments have been supplied, this is the first chance
// to make assertions on positional argument indexes
//
// Firt we verify that the index highest supplied index, is equal to the number of
// First we verify that the index highest supplied index, is equal to the number of
// positional arguments to verify there are no gaps (i.e. supplying an index of 1 and 3
// but no 2)

// #[cfg(feature = "vec_map")]
// fn _highest_idx(map: &VecMap<&str>) -> usize { map.keys().last().unwrap_or(0) }

// #[cfg(not(feature = "vec_map"))]
// fn _highest_idx(map: &VecMap<&str>) -> usize { *map.keys().last().unwrap_or(&0) }

let highest_idx = *self
.app
.args
Expand Down
92 changes: 0 additions & 92 deletions src/util/map.rs

This file was deleted.

4 changes: 2 additions & 2 deletions src/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ mod argstr;
mod fnv;
mod graph;
mod id;
mod map;
mod strext;

pub use self::fnv::Key;

pub(crate) use self::{argstr::ArgStr, graph::ChildGraph, id::Id, map::VecMap};
pub(crate) use self::{argstr::ArgStr, graph::ChildGraph, id::Id};
pub(crate) use vec_map::VecMap;

#[cfg(feature = "color")]
pub(crate) use termcolor;
Expand Down

0 comments on commit 739e704

Please sign in to comment.