Check bounds when calling Comment::get_child (fix #142) #164
Conversation
|
|
| } else { | ||
| unsafe { | ||
| Some(Comment { x: clang_Comment_getChild(self.x, idx) }) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm very new to rust, but my understanding is that unsafe code should be as limited as possible.
I see quite a few different practices in this file, but would it be sensible/work to have something like this?
Some( unsafe { Comment { x: clang_Comment_getChild(self.x, idx) } } )
Or
Some( Comment { x: unsafe { clang_Comment_getChild(self.x, idx) } } )
There was a problem hiding this comment.
Yeah, ideally, your later example is preferable. In this case it's not a big deal though, since there's no extra potentially unsafe stuff going on in that block.
There was a problem hiding this comment.
Looks good to me, just one nit. Also, feel free to address @jeanphilippeD comment if you wish.
Thanks!
| @@ -153,7 +153,8 @@ impl Annotations { | |||
| } | |||
|
|
|||
| for i in 0..comment.num_children() { | |||
| self.parse(&comment.get_child(i), matched); | |||
| let child = &comment.get_child(i).expect("index higher than number of children"); | |||
There was a problem hiding this comment.
Use unwrap, please, we can be sure we're inside the bounds.
|
Squash your commits, please. |
|
Nevermind, I've squashed them for you. @bors-servo r+ |
|
📌 Commit 449e101 has been approved by |
|
☀️ Test successful - status-travis |
Fixes #142.