Merge #115

115: Write Builders manually 2 r=Emilgardis a=burrbull

#101  without changes in Error hierarchy.

Should slightly be easier to review.

Co-authored-by: Andrey Zgarbul <[email protected]>

Author: bors[bot]

CHANGELOG.md

@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased]
 
+- Builder pattern now is used for creating structures
+- Peripherals are processed in parallel with `rayon`
+- Serde now skips empty tags
+- Fix: bug in `addressUnitBits`
+
 ## [v0.9.0] - 2019-11-17
 
 - [breaking-change]  make `ClusterInfo` `description` optional

src/error.rs

@@ -41,6 +41,14 @@ pub enum SVDError {
     EncodeNotImplemented(String),
     #[error("Error parsing SVD XML")]
     FileParseError,
+    #[error("Device must contain at least one peripheral")]
+    EmptyDevice,
+    #[error("Peripheral have `registers` tag, but it is empty")]
+    EmptyRegisters,
+    #[error("Cluster must contain at least one Register or Cluster")]
+    EmptyCluster,
+    #[error("Register have `fields` tag, but it is empty")]
+    EmptyFields,
 }
 
 // TODO: Consider making into an Error
@@ -49,4 +57,42 @@ pub enum InvalidBitRange {
     Syntax,
     ParseError,
     MsbLsb,
+    Empty,
+}
+
+#[derive(Clone, Debug, PartialEq, Eq, thiserror::Error)]
+pub enum BuildError {
+    #[error("`{0}` must be initialized")]
+    Uninitialized(String),
+}
+
+#[derive(Clone, Debug, PartialEq, Eq, thiserror::Error)]
+pub enum NameError {
+    #[error("Name `{0}` in tag `{1}` contains unexpected symbol")]
+    Invalid(String, String),
+}
+
+pub(crate) fn is_valid_name(name: &str) -> bool {
+    let mut chars = name.chars();
+    if let Some(first) = chars.next() {
+        if !(first.is_ascii_alphabetic() || first == '_') {
+            return false;
+        }
+        for c in chars {
+            if !(c.is_ascii_alphanumeric() || c == '_' || c == '%') {
+                return false;
+            }
+        }
+        true
+    } else {
+        false
+    }
+}
+
+pub(crate) fn check_name(name: &str, tag: &str) -> Result<()> {
+    if is_valid_name(name) {
+        Ok(())
+    } else {
+        Err(NameError::Invalid(name.to_string(), tag.to_string()).into())
+    }
 }

src/svd/bitrange.rs

@@ -45,18 +45,18 @@ impl Parse for BitRange {
             let text = range
                 .text
                 .as_ref()
-                .ok_or_else(|| anyhow::anyhow!("text missing"))?; // TODO: Make into a proper error, text empty or something similar
-                                                                  // TODO: If the `InvalidBitRange` enum was an error we could context into here somehow so that
-                                                                  // the output would be similar to the parse error
+                .ok_or_else(|| SVDError::InvalidBitRange(tree.clone(), InvalidBitRange::Empty))?;
             if !text.starts_with('[') {
                 return Err(
                     SVDError::InvalidBitRange(tree.clone(), InvalidBitRange::Syntax).into(),
-                ); // TODO: Maybe have a MissingOpen/MissingClosing variant
+                );
+                // TODO: Maybe have a MissingOpen/MissingClosing variant
             }
             if !text.ends_with(']') {
                 return Err(
                     SVDError::InvalidBitRange(tree.clone(), InvalidBitRange::Syntax).into(),
-                ); // TODO: Maybe have a MissingOpen/MissingClosing variant
+                );
+                // TODO: Maybe have a MissingOpen/MissingClosing variant
             }
 
             let mut parts = text[1..text.len() - 1].split(':');

src/svd/clusterinfo.rs

@@ -46,6 +46,81 @@ pub struct ClusterInfo {
     _extensible: (),
 }
 
+#[derive(Clone, Debug, Default, PartialEq)]
+pub struct ClusterInfoBuilder {
+    name: Option<String>,
+    address_offset: Option<u32>,
+    derived_from: Option<String>,
+    description: Option<String>,
+    header_struct_name: Option<String>,
+    default_register_properties: RegisterProperties,
+    children: Option<Vec<RegisterCluster>>,
+}
+
+impl ClusterInfoBuilder {
+    pub fn name(mut self, value: String) -> Self {
+        self.name = Some(value);
+        self
+    }
+    pub fn address_offset(mut self, value: u32) -> Self {
+        self.address_offset = Some(value);
+        self
+    }
+    pub fn derived_from(mut self, value: Option<String>) -> Self {
+        self.derived_from = value;
+        self
+    }
+    pub fn description(mut self, value: Option<String>) -> Self {
+        self.description = value;
+        self
+    }
+    pub fn header_struct_name(mut self, value: Option<String>) -> Self {
+        self.header_struct_name = value;
+        self
+    }
+    pub fn default_register_properties(mut self, value: RegisterProperties) -> Self {
+        self.default_register_properties = value;
+        self
+    }
+    pub fn children(mut self, value: Vec<RegisterCluster>) -> Self {
+        self.children = Some(value);
+        self
+    }
+    pub fn build(self) -> Result<ClusterInfo> {
+        (ClusterInfo {
+            name: self
+                .name
+                .ok_or_else(|| BuildError::Uninitialized("name".to_string()))?,
+            address_offset: self
+                .address_offset
+                .ok_or_else(|| BuildError::Uninitialized("address_offset".to_string()))?,
+            derived_from: self.derived_from,
+            description: self.description,
+            header_struct_name: self.header_struct_name,
+            default_register_properties: self.default_register_properties,
+            children: self
+                .children
+                .ok_or_else(|| BuildError::Uninitialized("children".to_string()))?,
+            _extensible: (),
+        })
+        .validate()
+    }
+}
+
+impl ClusterInfo {
+    fn validate(self) -> Result<Self> {
+        check_name(&self.name, "name")?;
+        if let Some(name) = self.derived_from.as_ref() {
+            check_name(name, "derivedFrom")?;
+        } else {
+            if self.children.is_empty() {
+                return Err(SVDError::EmptyCluster)?;
+            }
+        }
+        Ok(self)
+    }
+}
+
 impl Parse for ClusterInfo {
     type Object = Self;
     type Error = anyhow::Error;
@@ -58,24 +133,23 @@ impl Parse for ClusterInfo {
 
 impl ClusterInfo {
     fn _parse(tree: &Element, name: String) -> Result<Self> {
-        Ok(Self {
-            name, // TODO: Handle naming of cluster
-            derived_from: tree.attributes.get("derivedFrom").map(|s| s.to_owned()),
-            description: tree.get_child_text_opt("description")?,
-            header_struct_name: tree.get_child_text_opt("headerStructName")?,
-            address_offset: tree.get_child_u32("addressOffset")?,
-            default_register_properties: RegisterProperties::parse(tree)?,
-            children: {
+        ClusterInfoBuilder::default()
+            .name(name)
+            .derived_from(tree.attributes.get("derivedFrom").map(|s| s.to_owned()))
+            .description(tree.get_child_text_opt("description")?)
+            .header_struct_name(tree.get_child_text_opt("headerStructName")?)
+            .address_offset(tree.get_child_u32("addressOffset")?)
+            .default_register_properties(RegisterProperties::parse(tree)?)
+            .children({
                 let children: Result<Vec<_>, _> = tree
                     .children
                     .iter()
                     .filter(|t| t.name == "register" || t.name == "cluster")
                     .map(RegisterCluster::parse)
                     .collect();
                 children?
-            },
-            _extensible: (),
-        })
+            })
+            .build()
     }
 }
 

src/svd/cpu.rs

@@ -36,7 +36,84 @@ pub struct Cpu {
     pub has_vendor_systick: bool,
 
     // Reserve the right to add more fields to this struct
-    pub(crate) _extensible: (),
+    #[cfg_attr(feature = "serde", serde(skip))]
+    _extensible: (),
+}
+
+#[derive(Clone, Debug, Default, PartialEq)]
+pub struct CpuBuilder {
+    name: Option<String>,
+    revision: Option<String>,
+    endian: Option<Endian>,
+    mpu_present: Option<bool>,
+    fpu_present: Option<bool>,
+    nvic_priority_bits: Option<u32>,
+    has_vendor_systick: Option<bool>,
+}
+
+impl CpuBuilder {
+    pub fn name(mut self, value: String) -> Self {
+        self.name = Some(value);
+        self
+    }
+    pub fn revision(mut self, value: String) -> Self {
+        self.revision = Some(value);
+        self
+    }
+    pub fn endian(mut self, value: Endian) -> Self {
+        self.endian = Some(value);
+        self
+    }
+    pub fn mpu_present(mut self, value: bool) -> Self {
+        self.mpu_present = Some(value);
+        self
+    }
+    pub fn fpu_present(mut self, value: bool) -> Self {
+        self.fpu_present = Some(value);
+        self
+    }
+    pub fn nvic_priority_bits(mut self, value: u32) -> Self {
+        self.nvic_priority_bits = Some(value);
+        self
+    }
+    pub fn has_vendor_systick(mut self, value: bool) -> Self {
+        self.has_vendor_systick = Some(value);
+        self
+    }
+    pub fn build(self) -> Result<Cpu> {
+        (Cpu {
+            name: self
+                .name
+                .ok_or_else(|| BuildError::Uninitialized("name".to_string()))?,
+            revision: self
+                .revision
+                .ok_or_else(|| BuildError::Uninitialized("revision".to_string()))?,
+            endian: self
+                .endian
+                .ok_or_else(|| BuildError::Uninitialized("endian".to_string()))?,
+            mpu_present: self
+                .mpu_present
+                .ok_or_else(|| BuildError::Uninitialized("mpu_present".to_string()))?,
+            fpu_present: self
+                .fpu_present
+                .ok_or_else(|| BuildError::Uninitialized("fpu_present".to_string()))?,
+            nvic_priority_bits: self
+                .nvic_priority_bits
+                .ok_or_else(|| BuildError::Uninitialized("nvic_priority_bits".to_string()))?,
+            has_vendor_systick: self
+                .has_vendor_systick
+                .ok_or_else(|| BuildError::Uninitialized("has_vendor_systick".to_string()))?,
+            _extensible: (),
+        })
+        .validate()
+    }
+}
+
+impl Cpu {
+    fn validate(self) -> Result<Self> {
+        // TODO
+        Ok(self)
+    }
 }
 
 impl Parse for Cpu {
@@ -48,16 +125,15 @@ impl Parse for Cpu {
             return Err(SVDError::NameMismatch(tree.clone()).into());
         }
 
-        Ok(Self {
-            name: tree.get_child_text("name")?,
-            revision: tree.get_child_text("revision")?,
-            endian: Endian::parse(tree.get_child_elem("endian")?)?,
-            mpu_present: tree.get_child_bool("mpuPresent")?,
-            fpu_present: tree.get_child_bool("fpuPresent")?,
-            nvic_priority_bits: tree.get_child_u32("nvicPrioBits")?,
-            has_vendor_systick: tree.get_child_bool("vendorSystickConfig")?,
-            _extensible: (),
-        })
+        CpuBuilder::default()
+            .name(tree.get_child_text("name")?)
+            .revision(tree.get_child_text("revision")?)
+            .endian(Endian::parse(tree.get_child_elem("endian")?)?)
+            .mpu_present(tree.get_child_bool("mpuPresent")?)
+            .fpu_present(tree.get_child_bool("fpuPresent")?)
+            .nvic_priority_bits(tree.get_child_u32("nvicPrioBits")?)
+            .has_vendor_systick(tree.get_child_bool("vendorSystickConfig")?)
+            .build()
     }
 }
 
@@ -104,16 +180,16 @@ mod tests {
     #[test]
     fn decode_encode() {
         let tests = vec![(
-            Cpu {
-                name: String::from("EFM32JG12B500F512GM48"),
-                revision: String::from("5.1.1"),
-                endian: Endian::Little,
-                mpu_present: true,
-                fpu_present: true,
-                nvic_priority_bits: 8,
-                has_vendor_systick: false,
-                _extensible: (),
-            },
+            CpuBuilder::default()
+                .name("EFM32JG12B500F512GM48".to_string())
+                .revision("5.1.1".to_string())
+                .endian(Endian::Little)
+                .mpu_present(true)
+                .fpu_present(true)
+                .nvic_priority_bits(8)
+                .has_vendor_systick(false)
+                .build()
+                .unwrap(),
             "
                     <cpu>
                         <name>EFM32JG12B500F512GM48</name>