epage
commented
2 years ago Open epage opened 2 years ago
epage
commented
2 years ago epage
commented
2 years ago 
One possible soluion would be returning an array using const generics and using the 2021 edition into_iter to be able to return the actual value of the array without a need to clone.
At least in theorie.
epage
commented
2 years ago
@pksunkara posted the following snippet as one idea
trait ArgEnum<const COUNT: usize>: Sized {
fn variants() -> [Self; COUNT];
}
enum E {
One,
Two,
}
impl ArgEnum<2> for E {
fn variants() -> [Self; 2] {
[Self::One, Self::Two]
}
}
fn main() {}
@ModProg are you referring to using trait ArgEnum<const COUNT:usize>?
I see this is a trade off of making the trait harder to use vs requiring Clone.
Personally, I feel the Clone requirement is fairly minimal. Probably 99% of ArgEnums will be able to be Copy. On some occasions, someone will have a hidden variant. Among those using a hidden variant, what is the likelihood the type won't be Clone? My guess is fairly small. This is a niche of a niche that doesn't seem worth supporting. Releasing as-is, we can see what feedback we end up getting and re-examine this for clap4.
I recommend we close this, creating a new issue based on specific user feedback if it comes in.
epage
commented
2 years ago
@ModProg are you referring to using
trait ArgEnum<const COUNT:usize>?
yes.
I see this is a trade off of making the trait harder to use vs requiring Clone.
The only thing about this that makes me consider it is, that most people will only derive but never implement it, so this only affects a small number of people. And while the Clone is only a minor inconvenience it affects a lot more, who probably don't even use most of the features provided by my new implementation.
But I also see another implementation that just moves the to_string implementation back to the derive macro, that way we don't need either the Clone nor const generics.
epage
commented
2 years ago 
I agree with @ModProg regarding the estimates of how many use what kind of implementation.
But I also see another implementation that just moves the to_string implementation back to the derive macro, that way we don't need either the Clone nor const generics.
Do we have a clear example on how to achieve this?
epage
commented
2 years ago
This was the way it was done before, it was changed here: https://github.com/clap-rs/clap/pull/2762/commits/480035ac9c36490c4cccbf09e71b0ae84a6d527b#diff-1ea1dfdc52ed49bd532e4bd309c902a621e3bf02709b1fea86466c47fc7df7d9L118-L122
epage
commented
2 years ago
To clarify, I'm not concerned about the implementer. With that said, I have a PR out for one hand-written ArgEnum and expect to create another,. I also see more being written by crates that provide re-usable clap logic. Updating a length as code changes feels like busy work and is a breaking change for those crates. One the other hand, the cost of adding a #[derive(Clone)] is small, especially since people will most likely use it anyways.
However, creating a trait that can only be used in very specific ways because of the way the generic parameter is defined, severely limits our ability to evolve clap.
epage
commented
2 years ago
@ModProg, I thought the inherent from_str was a side benefit to us having to already make this change so that arg_values wouldn't return a Vec.
epage
commented
2 years ago
@ModProg, I thought the inherent
from_strwas a side benefit to us having to already make this change so thatarg_valueswouldn't return aVec.
It is, but it more enables the inherent from_str than requiring it.
The problem with the "generated" form is that we have to create the ArgValue for each call. tho I don't know if there is some optimazation happening because it technicly is a constant value even tho due to alias being a Vec we cannot declare it as const.
Another though is, we could also create the actual ArgValue during deriving, call get_name_and_aliases on it and put that value in the derived implementation. So it really is a constant array/slice for every variant.
epage
commented
2 years ago
The advantage to using the matches on ArgValue is, that it makes sure we use the same implementation in both the derived and build.
epage
commented
2 years ago 
I've only skimmed this issue, but in general I don't believe adding a const generic type argument to ArgEnum is worth the performance gain by not allocating a small Vec. My reasoning is clap still allocates, so it's not like we're removing all allocations. Additionally, Enum::values() would most likely only be called when either generating the help message, or other "generating" operation such as completion script, etc. I.e. it should not be allocating in the happy path of standard parsing. And even if it does we're still only talking a small Vec since allocation.
The cost however is a generic argument that's not entirely clear at first glance what it's for: Enum<3> doesn't mean a whole ton when the enum's variants are Foo, Bar, and Baz and it turns out that generic argument just so we can return an array size hint.
I would like to see improvement around ArgEnum for sure, but I think this is one of those issues that we can place on the back burner and garner some good ideas as v3 fleshes.
epage
commented
2 years ago
For comparison purposes
pub trait ArgEnum: Sized {
const VARIANTS: &'static [&'static str];
fn from_str(input: &str, case_insensitive: bool) -> Result<Self, String>;
fn as_arg(&self) -> Option<&'static str>;
}ArgValue support for native alias, hidden support, etcpub trait ArgEnum: Sized + Clone {
fn value_variants<'a>() -> &'a [Self];
fn from_str(input: &str, case_insensitive: bool) -> Result<Self, String>;
fn to_arg_value<'a>(&self) -> Option<ArgValue<'a>>;
}Clone
Copyhidden, and of those, they mostly likely support Clone, making the impact very smallVec Solutionpub trait ArgEnum: Sized {
fn variants() -> Vec<ArgValue<'static>>;
fn from_str(input: &str, case_insensitive: bool) -> Result<Self, String>;
fn as_arg(&self) -> Option<ArgValue<'static>>;
}Arg::new("foo").possible_values(arg_enum.variants()) does a throw-away allocation when buildingfrom_str would cause an allocation for parsing as well, since it'd call variantsconst Solutiontrait ArgEnum<const COUNT: usize>: Sized {
fn variants() -> [Self; COUNT];
fn from_str(input: &str, case_insensitive: bool) -> Result<Self, String>;
fn as_arg(&self) -> Option<ArgValue<'static>>;
}
2762 initially had common API calls for
ArgValueallocating aVec. We instead dropped that at the cost of adding theClonesuper trait.Can we do better?