Skip to content

Rust: Remove the need for segment_num#1

Closed
dotdash wants to merge 1 commit intoGNOME:rustificationfrom
dotdash:no_segment_num
Closed

Rust: Remove the need for segment_num#1
dotdash wants to merge 1 commit intoGNOME:rustificationfrom
dotdash:no_segment_num

Conversation

@dotdash
Copy link

@dotdash dotdash commented Oct 26, 2016

segment_num only kept track of the index of the last element in the
segments vector, but the vector already keeps track of that itself.
Using its last_mut() method instead, we can get rid of the counter
and its associated logic, and also get to use expect() with an
appropriate error message, instead of getting a plain "index out of
bounds" error when there is no segment yet.

`segment_num` only kept track of the index of the last element in the
`segments` vector, but the vector already keeps track of that itself.
Using its `last_mut()` method instead, we can get rid of the counter
and its associated logic, and also get to use `expect()` with an
appropriate error message, instead of getting a plain "index out of
bounds" error when there is no segment yet.
@federicomenaquintero
Copy link
Contributor

From your patch I learned both about last_mut() and about expect(), and I think I finally understood what Option/Some/None are about: basically, removing the need for null pointers in an elegant way. So, thanks for that!

I didn't see this pull request before continuing my refactoring (librsvg's development goes on in git.gnome.org, not in github, but I clearly need to start paying attention here!). I ended up doing something completely different, starting with converting the ugly struct Segment into an enum. This led to a chain of cleanups so that now segment_num is gone, and each individual match case is cleaner/shorter as well.

I'll start paying attention to pull requests here. Thanks again for looking into my 100% newbie Rust code :)

@masklinn
Copy link

masklinn commented Oct 27, 2016

@federicomenaquintero you may want to close the PR as well, since it doesn't apply anymore.

And for what it's worth the project was discussed on /r/rust (from your blog post)

@dotdash
Copy link
Author

dotdash commented Oct 27, 2016

Ah, ok, I just googled librsvg and github was the first hit that showed up WRT source code. Glad that my PR served as learning material :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants