Use is_default instead of is_empty or is_und for locale-ish types
#5359
Conversation
robertbastian
requested review from
Manishearth,
sffc,
zbraniecki,
nciric,
dminor,
nordzilla and
a team
as code owners
There was a problem hiding this comment.
This wasn't discussed in the issue, but I think it's only consistent to also call it is_default instead of is_empty for Language. And because clear logically requires its result to be "empty", I removed that method (it's unused by us and trivial anyway).
| @@ -34,7 +33,7 @@ fn overview_bench(c: &mut Criterion) { | |||
| // ranging from -1e9 to 1e9. | |||
| let fdf = FixedDecimalFormatter::try_new_unstable( | |||
| &provider, | |||
| &Locale::UND.into(), | |||
| &Default::default(), | |||
There was a problem hiding this comment.
Thought, here and elsewhere: Locale::UND.into() is more clear than Default::default(); I think &Locale::default().into() would be the most clear. I also don't know whether Preferences will impl Default. @zbraniecki might have thoughts on this.
There was a problem hiding this comment.
We use Default::default() in constructors basically everywhere in tests. I don't see any value in changing a handful of invocations to Locale::default().into() now, only to change it again when we have preferences. If preferences don't implement Default, we'll have to change these call sites then anyway, so this won't be forgotten, if they do, then Default::default() is perfectly fine.
Fixes #5199.