Fix take/into function names and conventions #5723
Conversation
Manishearth
requested review from
sffc,
robertbastian,
nciric,
dminor,
snktd and
a team
as code owners
Manishearth
force-pushed
the
take-into
branch
from
74654d3 to
6e6209c
Compare
| @@ -66,7 +66,7 @@ impl<'a> ParseState<'a> { | |||
|
|
|||
| /// Takes the set of keys in order of discovery and replaces it with an | |||
| /// empty set. | |||
| pub fn take_keys(self) -> IndexSet<Key<'a>> { | |||
| pub fn take_keys(&self) -> IndexSet<Key<'a>> { | |||
There was a problem hiding this comment.
huh, is this a take? does it use interior mutability?
There was a problem hiding this comment.
Looks like it has a RefCell.
Maybe this fn should be renamed into_keys() since it seems like better code style to not use the class after taking the items out of the RefCell.
@leftmostcat thoughts?
There was a problem hiding this comment.
Also it's supposed to be called after all of parsing is done so that Rc should probably be unique, we could maybe call mem::take(rc make_mut())? deal with the error case somehow.
There was a problem hiding this comment.
Merging but hopeful for a better answer for the resb parser.
| @@ -66,7 +66,7 @@ impl<'a> ParseState<'a> { | |||
|
|
|||
| /// Takes the set of keys in order of discovery and replaces it with an | |||
| /// empty set. | |||
| pub fn take_keys(self) -> IndexSet<Key<'a>> { | |||
| pub fn take_keys(&self) -> IndexSet<Key<'a>> { | |||
There was a problem hiding this comment.
Looks like it has a RefCell.
Maybe this fn should be renamed into_keys() since it seems like better code style to not use the class after taking the items out of the RefCell.
@leftmostcat thoughts?
Fixes #4668