Don't eagerly absolutize source file paths.

Author: reitermarkus

bindgen/clang.rs

@@ -6,8 +6,10 @@
 
 use crate::ir::context::{BindgenContext, IncludeLocation};
 use clang_sys::*;
+use std::borrow::Cow;
 use std::cmp;
 use std::convert::TryInto;
+use std::env::current_dir;
 use std::ffi::{CStr, CString};
 use std::fmt;
 use std::hash::Hash;
@@ -396,9 +398,8 @@ impl Cursor {
                     offset: offset.try_into().unwrap(),
                 }
             } else {
-                let file_name = cxstring_into_string(clang_getFileName(file));
                 SourceLocation::File {
-                    file_path: absolutize_path(file_name),
+                    file: SourceFile::from_raw(file),
                     line: line.try_into().unwrap(),
                     column: column.try_into().unwrap(),
                     offset: offset.try_into().unwrap(),
@@ -536,12 +537,8 @@ impl Cursor {
         let mut children = self.collect_children();
         for child in &children {
             if child.kind() == CXCursor_InclusionDirective {
-                if let Some(included_file_name) = child.get_included_file_name()
-                {
-                    ctx.add_include(
-                        absolutize_path(included_file_name),
-                        child.location(),
-                    );
+                if let Some(included_file) = child.get_included_file() {
+                    ctx.add_include(included_file, child.location());
                 }
             }
         }
@@ -934,14 +931,12 @@ impl Cursor {
     /// Obtain the real path name of a cursor of InclusionDirective kind.
     ///
     /// Returns None if the cursor does not include a file, otherwise the file's full name
-    pub(crate) fn get_included_file_name(&self) -> Option<String> {
-        let file = unsafe { clang_sys::clang_getIncludedFile(self.x) };
+    pub(crate) fn get_included_file(&self) -> Option<SourceFile> {
+        let file = unsafe { clang_getIncludedFile(self.x) };
         if file.is_null() {
             None
         } else {
-            Some(unsafe {
-                cxstring_into_string(clang_sys::clang_getFileName(file))
-            })
+            Some(unsafe { SourceFile::from_raw(file) })
         }
     }
 }
@@ -1579,8 +1574,8 @@ pub(crate) enum SourceLocation {
     },
     /// Location in a source file.
     File {
-        /// Name of the source file.
-        file_path: PathBuf,
+        /// The source file.
+        file: SourceFile,
         /// Line in the source file.
         line: usize,
         /// Column in the source file.
@@ -1590,18 +1585,6 @@ pub(crate) enum SourceLocation {
     },
 }
 
-fn absolutize_path<P: AsRef<Path>>(path: P) -> PathBuf {
-    let path = path.as_ref();
-
-    if path.is_relative() {
-        std::env::current_dir()
-            .expect("Cannot retrieve current directory")
-            .join(path)
-    } else {
-        path.to_owned()
-    }
-}
-
 impl SourceLocation {
     /// Locations of built-in items provided by the compiler (which don't have a source file),
     /// are sorted first. Remaining locations are sorted by their position in the source file.
@@ -1625,59 +1608,64 @@ impl SourceLocation {
                 other.cmp_by_source_order(self, ctx).reverse()
             }
             (
+                SourceLocation::File { file, offset, .. },
                 SourceLocation::File {
-                    file_path, offset, ..
-                },
-                SourceLocation::File {
-                    file_path: other_file_path,
+                    file: other_file,
                     offset: other_offset,
                     ..
                 },
             ) => {
+                let file_path = file.path();
+                let other_file_path = other_file.path();
+
                 if file_path == other_file_path {
                     return offset.cmp(other_offset);
                 }
 
                 // If `file` is transitively included via `ancestor_file`,
                 // find the offset of the include directive in `ancestor_file`.
-                let offset_in_ancestor = |file: &Path, ancestor_file: &Path| {
-                    let mut file = file;
-                    while file != ancestor_file {
-                        let include_location = ctx.include_location(file);
-                        file = if let IncludeLocation::File {
-                            file_path: file,
-                            offset,
-                            ..
-                        } = include_location
-                        {
-                            if file == ancestor_file {
-                                return Some(offset);
+                let offset_in_ancestor =
+                    |file_path: &Path, ancestor_file_path: &Path| {
+                        let mut file_path = Cow::Borrowed(file_path);
+                        while file_path != ancestor_file_path {
+                            let include_location =
+                                ctx.include_location(file_path);
+                            file_path = if let IncludeLocation::File {
+                                file,
+                                offset,
+                                ..
+                            } = include_location
+                            {
+                                let file_path = Cow::Owned(file.path());
+
+                                if file_path == ancestor_file_path {
+                                    return Some(offset);
+                                }
+
+                                file_path
+                            } else {
+                                break;
                             }
-
-                            file
-                        } else {
-                            break;
                         }
-                    }
 
-                    None
-                };
+                        None
+                    };
 
                 if let Some(offset) =
-                    offset_in_ancestor(file_path, other_file_path)
+                    offset_in_ancestor(&file_path, &other_file_path)
                 {
                     return offset.cmp(other_offset);
                 }
 
                 if let Some(other_offset) =
-                    offset_in_ancestor(other_file_path, file_path)
+                    offset_in_ancestor(&other_file_path, &file_path)
                 {
                     return offset.cmp(other_offset);
                 }
 
                 // If the source files are siblings, compare their include locations.
                 let parent = ctx.include_location(file_path);
-                let other_parent = ctx.include_location(other_file_path);
+                let other_parent = ctx.include_location(&other_file_path);
                 parent.cmp_by_source_order(other_parent, ctx)
             }
         }
@@ -1689,11 +1677,8 @@ impl fmt::Display for SourceLocation {
         match self {
             Self::Builtin { .. } => "built-in".fmt(f),
             Self::File {
-                file_path,
-                line,
-                column,
-                ..
-            } => write!(f, "{}:{}:{}", file_path.display(), line, column),
+                file, line, column, ..
+            } => write!(f, "{}:{}:{}", file.path().display(), line, column),
         }
     }
 }
@@ -1803,17 +1788,63 @@ impl Iterator for CommentAttributesIterator {
     }
 }
 
-fn cxstring_to_string_leaky(s: CXString) -> String {
+/// A source file.
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
+pub(crate) struct SourceFile {
+    name: Box<str>,
+}
+
+impl SourceFile {
+    /// Creates a new `SourceFile` from a file name.
+    #[inline]
+    pub fn new<P: AsRef<str>>(name: P) -> Self {
+        Self {
+            name: name.as_ref().to_owned().into_boxed_str(),
+        }
+    }
+
+    /// Creates a new `SourceFile` from a raw pointer.
+    ///
+    /// # Safety
+    ///
+    /// `file` must point to a valid `CXFile`.
+    pub unsafe fn from_raw(file: CXFile) -> Self {
+        let name = unsafe { cxstring_into_string(clang_getFileName(file)) };
+
+        Self::new(name)
+    }
+
+    #[inline]
+    pub fn name(&self) -> &str {
+        &self.name
+    }
+
+    /// Get the path of this source file.
+    pub fn path(&self) -> PathBuf {
+        let path = Path::new(self.name());
+        if path.is_relative() {
+            current_dir()
+                .expect("Cannot retrieve current directory")
+                .join(path)
+        } else {
+            path.to_owned()
+        }
+    }
+}
+
+unsafe fn cxstring_to_string_leaky(s: CXString) -> String {
     if s.data.is_null() {
-        return "".to_owned();
+        Cow::Borrowed("")
+    } else {
+        let c_str = CStr::from_ptr(clang_getCString(s) as *const _);
+        c_str.to_string_lossy()
     }
-    let c_str = unsafe { CStr::from_ptr(clang_getCString(s) as *const _) };
-    c_str.to_string_lossy().into_owned()
+    .into_owned()
 }
 
-fn cxstring_into_string(s: CXString) -> String {
+unsafe fn cxstring_into_string(s: CXString) -> String {
     let ret = cxstring_to_string_leaky(s);
-    unsafe { clang_disposeString(s) };
+    clang_disposeString(s);
     ret
 }
 
@@ -1924,13 +1955,13 @@ impl TranslationUnit {
         }
     }
 
-    /// Get the source file path of this translation unit.
-    pub(crate) fn path(&self) -> PathBuf {
-        let file_name = unsafe {
+    /// Get the source file of this.
+    pub fn file(&self) -> SourceFile {
+        let name = unsafe {
             cxstring_into_string(clang_getTranslationUnitSpelling(self.x))
         };
 
-        absolutize_path(file_name)
+        SourceFile::new(name)
     }
 
     /// Is this the null translation unit?

bindgen/codegen/mod.rs

@@ -4379,12 +4379,13 @@ fn unsupported_abi_diagnostic(
         );
 
         if let Some(crate::clang::SourceLocation::File {
-            file_path,
+            file,
             line,
             column,
             ..
         }) = location.cloned()
         {
+            let file_path = file.path();
             if let Ok(Some(source)) = get_line(&file_path, line) {
                 let mut slice = Slice::default();
                 slice
@@ -4420,12 +4421,13 @@ fn variadic_fn_diagnostic(
             .add_annotation("No code will be generated for this function.", Level::Note);
 
         if let Some(crate::clang::SourceLocation::File {
-            file_path,
+            file,
             line,
             column,
             ..
         }) = location.cloned()
         {
+            let file_path = file.path();
             if let Ok(Some(source)) = get_line(&file_path, line) {
                 let mut slice = Slice::default();
                 slice

bindgen/deps.rs

@@ -1,24 +1,26 @@
 /// Generating build depfiles from parsed bindings.
 use std::{collections::BTreeSet, path::PathBuf};
 
+use crate::clang::SourceFile;
+
 #[derive(Clone, Debug)]
 pub(crate) struct DepfileSpec {
     pub output_module: String,
     pub depfile_path: PathBuf,
 }
 
 impl DepfileSpec {
-    pub fn write(&self, deps: &BTreeSet<Box<str>>) -> std::io::Result<()> {
+    pub fn write(&self, deps: &BTreeSet<SourceFile>) -> std::io::Result<()> {
         std::fs::write(&self.depfile_path, self.to_string(deps))
     }
 
-    fn to_string(&self, deps: &BTreeSet<Box<str>>) -> String {
+    fn to_string(&self, deps: &BTreeSet<SourceFile>) -> String {
         // Transforms a string by escaping spaces and backslashes.
         let escape = |s: &str| s.replace('\\', "\\\\").replace(' ', "\\ ");
 
         let mut buf = format!("{}:", escape(&self.output_module));
         for file in deps {
-            buf = format!("{} {}", buf, escape(file));
+            buf = format!("{} {}", buf, escape(file.name()));
         }
         buf
     }
@@ -36,13 +38,13 @@ mod tests {
         };
 
         let deps: BTreeSet<_> = vec![
-            r"/absolute/path".into(),
-            r"C:\win\absolute\path".into(),
-            r"../relative/path".into(),
-            r"..\win\relative\path".into(),
-            r"../path/with spaces/in/it".into(),
-            r"..\win\path\with spaces\in\it".into(),
-            r"path\with/mixed\separators".into(),
+            SourceFile::new(r"/absolute/path"),
+            SourceFile::new(r"C:\win\absolute\path"),
+            SourceFile::new(r"../relative/path"),
+            SourceFile::new(r"..\win\relative\path"),
+            SourceFile::new(r"../path/with spaces/in/it"),
+            SourceFile::new(r"..\win\path\with spaces\in\it"),
+            SourceFile::new(r"path\with/mixed\separators"),
         ]
         .into_iter()
         .collect();