Thanks for taking on this !
I have only one minor remark below
On 12/5/18 2:43 PM, Yuya Nishihara wrote:
> # HG changeset patch
> # User Yuya Nishihara <y...@tcha.org>
> # Date 1540817453 -32400
> # Mon Oct 29 21:50:53 2018 +0900
> # Node ID 3afbe6d40d6859d313f45b66ad5ca3c0a48f2e06
> # Parent f5cdfa49994e3943ba7c4ce2d66708142f0c7058
> rust: propagate error of index_get_parents() properly
>
> Before, rustla_contains() would return 0 on error, and the exception would
> be cleared or noticed somewhere else. We need to propagate the error from
> AncestorsIterator up to the FFI surface.
>
> diff --git a/rust/hg-core/src/ancestors.rs b/rust/hg-core/src/ancestors.rs
> --- a/rust/hg-core/src/ancestors.rs
> +++ b/rust/hg-core/src/ancestors.rs
> @@ -77,19 +77,20 @@ impl<G: Graph> AncestorsIterator<G> {
> /// is in the ancestors it emits.
> /// This is meant for iterators actually dedicated to that kind of
> /// purpose
> - pub fn contains(&mut self, target: Revision) -> bool {
> + pub fn contains(&mut self, target: Revision) -> Result<bool, GraphError>
> {
> if self.seen.contains(&target) && target != NULL_REVISION {
> - return true;
> + return Ok(true);
> }
> - for rev in self {
> + for item in self {
> + let rev = item?;
> if rev == target {
> - return true;
> + return Ok(true);
> }
> if rev < target {
> - return false;
> + return Ok(false);
> }
> }
> - false
> + Ok(false)
> }
> }
>
> @@ -117,19 +118,19 @@ impl<G: Graph> AncestorsIterator<G> {
> /// concrete caller we target, so we shouldn't need a finer error treatment
> /// for the time being.
I think the comment right above about error treatment becomes obsolete
with this patch.
> impl<G: Graph> Iterator for AncestorsIterator<G> {
> - type Item = Revision;
> + type Item = Result<Revision, GraphError>;
>
> - fn next(&mut self) -> Option<Revision> {
> + fn next(&mut self) -> Option<Self::Item> {
> let current = match self.visit.peek() {
> None => {
> return None;
> }
> Some(c) => *c,
> };
> - let (p1, p2) = self
> - .graph
> - .parents(current)
> - .unwrap_or((NULL_REVISION, NULL_REVISION));
> + let (p1, p2) = match self.graph.parents(current) {
> + Ok(ps) => ps,
> + Err(e) => return Some(Err(e)),
> + };
> if p1 < self.stoprev || self.seen.contains(&p1) {
> self.visit.pop();
> } else {
> @@ -138,7 +139,7 @@ impl<G: Graph> Iterator for AncestorsIte
> };
>
> self.conditionally_push_rev(p2);
> - Some(current)
> + Some(Ok(current))
> }
> }
>
> diff --git a/rust/hg-direct-ffi/src/ancestors.rs
> b/rust/hg-direct-ffi/src/ancestors.rs
> --- a/rust/hg-direct-ffi/src/ancestors.rs
> +++ b/rust/hg-direct-ffi/src/ancestors.rs
> @@ -139,7 +139,11 @@ pub extern "C" fn rustlazyancestors_next
> #[inline]
> fn raw_next<G: Graph>(raw: *mut AncestorsIterator<G>) -> c_long {
> let as_ref = unsafe { &mut *raw };
> - as_ref.next().unwrap_or(NULL_REVISION) as c_long
> + let rev = match as_ref.next() {
> + Some(Ok(rev)) => rev,
> + Some(Err(_)) | None => NULL_REVISION,
> + };
> + rev as c_long
> }
>
> #[no_mangle]
> @@ -157,10 +161,10 @@ fn raw_contains<G: Graph>(
> target: c_long,
> ) -> c_int {
> let as_ref = unsafe { &mut *raw };
> - if as_ref.contains(target as Revision) {
> - return 1;
> + match as_ref.contains(target as Revision) {
> + Ok(r) => r as c_int,
> + Err(_) => -1,
> }
> - 0
> }
>
> #[cfg(test)]
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
--
Georges Racinet
Anybox SAS, http://anybox.fr
Téléphone: +33 6 51 32 07 27
GPG: B59E 22AB B842 CAED 77F7 7A7F C34F A519 33AB 0A35, sur serveurs publics
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
