Moving complex segmenter logic into complex module
#3349
Conversation
robertbastian
requested review from
aethanyc,
makotokato and
sffc
as code owners
robertbastian
requested review from
Manishearth
and removed request for
aethanyc and
makotokato
| dict: &'l UCharDictionaryBreakDataV1<'l>, | ||
| grapheme: &'l RuleBreakDataV1<'l>, | ||
| } | ||
|
|
||
| impl<'l> DictionarySegmenter<'l> { | ||
| pub fn new( | ||
| pub(super) fn new( | ||
| dict: &'l DataPayload<UCharDictionaryBreakDataV1Marker>, |
There was a problem hiding this comment.
Nit (optional): Can you use this PR to address my unresolved comment from #3348
Nit: You should almost never have a
&'a DataPayload<FooV1Marker>you should instead have a
&'a FooV1<'a>
There was a problem hiding this comment.
I don't think this is the place
There was a problem hiding this comment.
Actually in this instance it really complicates things. Instead of being able to .map(Result::as_ref) I'd have to have a map that unpacks both values, and have that be conditional on the LSTM being the never type.
| self.data | ||
| } | ||
|
|
||
| pub fn copy_submatrix<const M: usize>(&mut self, from: usize, to: usize) { | ||
| pub(super) fn copy_submatrix<const M: usize>(&mut self, from: usize, to: usize) { |
There was a problem hiding this comment.
Nit: It seems more verbose to do this to all the methods. If you want to be explicit that it is pub(super),just label the class as such, not the individual methods on the class
There was a problem hiding this comment.
I prefer it that way, because it makes the semver relevance clearer without hunting for the struct's definition.
Based on #3348
Makes it clearer where the modules are used, and makes a whole lot of types more private.