clang: Clean up source order sorting.

This doesn't change behavior but is easier to debug and reason about (because you have the relevant cursors there).
emilio · Jun 15, 2023 · 894535e · 894535e
1 parent 264075a
commit 894535e
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 73 deletions.
diff --git a/bindgen/clang.rs b/bindgen/clang.rs
@@ -498,23 +498,21 @@ impl Cursor {
         }
     }
 
-    /// Traverse this curser's children, sorted by where they appear in source code.
+    /// Traverse all of this cursor's children, sorted by where they appear in source code.
     ///
     /// Call the given function on each AST node traversed.
     pub(crate) fn visit_sorted<Visitor>(
         &self,
         ctx: &mut BindgenContext,
         mut visitor: Visitor,
     ) where
-        Visitor: FnMut(&mut BindgenContext, Cursor) -> CXChildVisitResult,
+        Visitor: FnMut(&mut BindgenContext, Cursor),
     {
         let mut children = self.collect_children();
-
         for child in &children {
             if child.kind() == CXCursor_InclusionDirective {
                 if let Some(included_file) = child.get_included_file_name() {
                     let location = child.location();
-
                     let (source_file, _, _, offset) = location.location();
 
                     if let Some(source_file) = source_file.name() {
@@ -523,19 +521,62 @@ impl Cursor {
                 }
             }
         }
-
-        children.sort_by(|child1, child2| {
-            child1
-                .location()
-                .partial_cmp_with_context(&child2.location(), ctx)
-                .unwrap_or(std::cmp::Ordering::Equal)
-        });
-
+        children
+            .sort_by(|child1, child2| child1.cmp_by_source_order(child2, ctx));
         for child in children {
             visitor(ctx, child);
         }
     }
 
+    /// Compare source order of two cursors, considering `#include` directives.
+    ///
+    /// Built-in items provided by the compiler (which don't have a source file),
+    /// are sorted first. Remaining files are sorted by their position in the source file.
+    /// If the items' source files differ, they are sorted by the position of the first
+    /// `#include` for their source file. If no source files are included, `None` is returned.
+    fn cmp_by_source_order(
+        &self,
+        other: &Self,
+        ctx: &BindgenContext,
+    ) -> cmp::Ordering {
+        let (file, _, _, offset) = self.location().location();
+        let (other_file, _, _, other_offset) = other.location().location();
+
+        let (file, other_file) = match (file.name(), other_file.name()) {
+            (Some(file), Some(other_file)) => (file, other_file),
+            // Built-in definitions should come first.
+            (Some(_), None) => return cmp::Ordering::Greater,
+            (None, Some(_)) => return cmp::Ordering::Less,
+            (None, None) => return cmp::Ordering::Equal,
+        };
+
+        if file == other_file {
+            // Both items are in the same source file, compare by byte offset.
+            return offset.cmp(&other_offset);
+        }
+
+        let include_location = ctx.included_file_location(&file);
+        let other_include_location = ctx.included_file_location(&other_file);
+        match (include_location, other_include_location) {
+            (Some((file2, offset2)), _) if file2 == other_file => {
+                offset2.cmp(&other_offset)
+            }
+            (Some(_), None) => cmp::Ordering::Greater,
+            (_, Some((other_file2, other_offset2))) if file == other_file2 => {
+                offset.cmp(&other_offset2)
+            }
+            (None, Some(_)) => cmp::Ordering::Less,
+            (Some((file2, offset2)), Some((other_file2, other_offset2))) => {
+                if file2 == other_file2 {
+                    offset2.cmp(&other_offset2)
+                } else {
+                    cmp::Ordering::Equal
+                }
+            }
+            (None, None) => cmp::Ordering::Equal,
+        }
+    }
+
     /// Collect all of this cursor's children into a vec and return them.
     pub(crate) fn collect_children(&self) -> Vec<Cursor> {
         let mut children = vec![];
@@ -1564,63 +1605,6 @@ impl SourceLocation {
     }
 }
 
-impl SourceLocation {
-    /// Compare source locations, also considering `#include` directives.
-    ///
-    /// Built-in items provided by the compiler (which don't have a source file),
-    /// are sorted first. Remaining files are sorted by their position in the source file.
-    /// If the items' source files differ, they are sorted by the position of the first
-    /// `#include` for their source file. If no source files are included, `None` is returned.
-    pub(crate) fn partial_cmp_with_context(
-        &self,
-        other: &Self,
-        ctx: &BindgenContext,
-    ) -> Option<cmp::Ordering> {
-        let (file, _, _, offset) = self.location();
-        let (other_file, _, _, other_offset) = other.location();
-
-        match (file.name(), other_file.name()) {
-            (Some(file), Some(other_file)) => {
-                if file == other_file {
-                    return offset.partial_cmp(&other_offset);
-                }
-
-                let include_location = ctx.included_file_location(&file);
-                let other_include_location =
-                    ctx.included_file_location(&other_file);
-
-                match (include_location, other_include_location) {
-                    (Some((file2, offset2)), _) if file2 == other_file => {
-                        offset2.partial_cmp(&other_offset)
-                    }
-                    (Some(_), None) => Some(cmp::Ordering::Greater),
-                    (_, Some((other_file2, other_offset2)))
-                        if file == other_file2 =>
-                    {
-                        offset.partial_cmp(&other_offset2)
-                    }
-                    (None, Some(_)) => Some(cmp::Ordering::Less),
-                    (
-                        Some((file2, offset2)),
-                        Some((other_file2, other_offset2)),
-                    ) => {
-                        if file2 == other_file2 {
-                            offset2.partial_cmp(&other_offset2)
-                        } else {
-                            None
-                        }
-                    }
-                    (None, None) => Some(cmp::Ordering::Equal),
-                }
-            }
-            // Built-in definitions should come first.
-            (Some(_), None) => Some(cmp::Ordering::Greater),
-            (None, Some(_)) => Some(cmp::Ordering::Less),
-            (None, None) => Some(cmp::Ordering::Equal),
-        }
-    }
-}
-
 impl fmt::Display for SourceLocation {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         let (file, line, col, _) = self.location();

diff --git a/bindgen/lib.rs b/bindgen/lib.rs
@@ -1084,12 +1084,11 @@ fn parse_one(
     ctx: &mut BindgenContext,
     cursor: clang::Cursor,
     parent: Option<ItemId>,
-) -> clang_sys::CXChildVisitResult {
+) {
     if !filter_builtins(ctx, &cursor) {
-        return CXChildVisit_Continue;
+        return;
     }
 
-    use clang_sys::CXChildVisit_Continue;
     match Item::parse(cursor, parent, ctx) {
         Ok(..) => {}
         Err(ParseError::Continue) => {}
@@ -1098,7 +1097,6 @@ fn parse_one(
                 .visit_sorted(ctx, |ctx, child| parse_one(ctx, child, parent));
         }
     }
-    CXChildVisit_Continue
 }
 
 /// Parse the Clang AST into our `Item` internal representation.