Reinstate Option over ViolationFn, drop NoOp, decentralize

This moves some logically ViolationFn functions back into the Parser
and further spreads the use of `if let Some(vfn)`. It does however
reduce the total number of changes for the feature.

Author: dekellum

src/lib.rs

@@ -186,7 +186,7 @@ impl HeapSizeOf for Url {
 pub struct ParseOptions<'a> {
     base_url: Option<&'a Url>,
     encoding_override: encoding::EncodingOverride,
-    violation_fn: ViolationFn<'a>,
+    violation_fn: Option<ViolationFn<'a>>,
 }
 
 impl<'a> ParseOptions<'a> {
@@ -216,10 +216,7 @@ impl<'a> ParseOptions<'a> {
     /// passed to either method will be used by a parser.
     #[deprecated]
     pub fn log_syntax_violation(mut self, new: Option<&'a Fn(&'static str)>) -> Self {
-        self.violation_fn = match new {
-            Some(f) => ViolationFn::OldFn(f),
-            None => ViolationFn::NoOp
-        };
+        self.violation_fn = new.map(|f| ViolationFn::OldFn(f));
         self
     }
 
@@ -246,10 +243,7 @@ impl<'a> ParseOptions<'a> {
     /// # run().unwrap();
     /// ```
     pub fn syntax_violation_callback(mut self, new: Option<&'a Fn(SyntaxViolation)>) -> Self {
-        self.violation_fn = match new {
-            Some(f) => ViolationFn::NewFn(f),
-            None => ViolationFn::NoOp
-        };
+        self.violation_fn = new.map(|f| ViolationFn::NewFn(f));
         self
     }
 
@@ -402,7 +396,7 @@ impl Url {
         ParseOptions {
             base_url: None,
             encoding_override: EncodingOverride::utf8(),
-            violation_fn: ViolationFn::NoOp,
+            violation_fn: None,
         }
     }
 

src/parser.rs

@@ -160,12 +160,12 @@ pub struct Input<'i> {
 
 impl<'i> Input<'i> {
     pub fn new(input: &'i str) -> Self {
-        Input::with_log(input, ViolationFn::NoOp)
+        Input::with_log(input, None)
     }
 
-    pub fn with_log(original_input: &'i str, vfn: ViolationFn) -> Self {
+    pub fn with_log(original_input: &'i str, vfn: Option<ViolationFn>) -> Self {
         let input = original_input.trim_matches(c0_control_or_space);
-        if vfn.is_set() {
+        if let Some(vfn) = vfn {
             if input.len() < original_input.len() {
                 vfn.call(SyntaxViolation::C0SpaceIgnored)
             }
@@ -268,7 +268,6 @@ impl<'i> Iterator for Input<'i> {
 pub enum ViolationFn<'a> {
     NewFn(&'a (Fn(SyntaxViolation) + 'a)),
     OldFn(&'a (Fn(&'static str) + 'a)),
-    NoOp
 }
 
 impl<'a> ViolationFn<'a> {
@@ -277,27 +276,6 @@ impl<'a> ViolationFn<'a> {
         match self {
             ViolationFn::NewFn(f) => f(v),
             ViolationFn::OldFn(f) => f(v.description()),
-            ViolationFn::NoOp => {}
-        }
-    }
-
-    /// Call with a violation, if provided test returns true. Avoids
-    /// the test entirely if `NoOp`.
-    pub fn call_if<F>(self, v: SyntaxViolation, test: F)
-        where F: Fn() -> bool
-    {
-        match self {
-            ViolationFn::NewFn(f) => if test() { f(v) },
-            ViolationFn::OldFn(f) => if test() { f(v.description()) },
-            ViolationFn::NoOp => {} // avoid test
-        }
-    }
-
-    /// True if not `NoOp`
-    pub fn is_set(self) -> bool {
-        match self {
-            ViolationFn::NoOp => false,
-            _ => true
         }
     }
 }
@@ -307,7 +285,6 @@ impl<'a> fmt::Debug for ViolationFn<'a> {
         match *self {
             ViolationFn::NewFn(_) => write!(f, "NewFn(Fn(SyntaxViolation))"),
             ViolationFn::OldFn(_) => write!(f, "OldFn(Fn(&'static str))"),
-            ViolationFn::NoOp     => write!(f, "NoOp")
         }
     }
 }
@@ -316,7 +293,7 @@ pub struct Parser<'a> {
     pub serialization: String,
     pub base_url: Option<&'a Url>,
     pub query_encoding_override: EncodingOverride,
-    pub violation_fn: ViolationFn<'a>,
+    pub violation_fn: Option<ViolationFn<'a>>,
     pub context: Context,
 }
 
@@ -333,11 +310,26 @@ impl<'a> Parser<'a> {
             serialization: serialization,
             base_url: None,
             query_encoding_override: EncodingOverride::utf8(),
-            violation_fn: ViolationFn::NoOp,
+            violation_fn: None,
             context: Context::Setter,
         }
     }
 
+    fn syntax_violation(&self, v: SyntaxViolation) {
+        if let Some(vfn) = self.violation_fn {
+            vfn.call(v)
+        }
+    }
+
+    fn syntax_violation_if<F: Fn() -> bool>(&self, v: SyntaxViolation, test: F) {
+        // Skip test if not logging.
+        if let Some(vfn) = self.violation_fn {
+            if test() {
+                vfn.call(v)
+            }
+        }
+    }
+
     /// https://url.spec.whatwg.org/#concept-basic-url-parser
     pub fn parse_url(mut self, input: &str) -> ParseResult<Url> {
         let input = Input::with_log(input, self.violation_fn);
@@ -397,7 +389,7 @@ impl<'a> Parser<'a> {
         self.serialization.push(':');
         match scheme_type {
             SchemeType::File => {
-                self.violation_fn.call_if(ExpectedFileDoubleSlash, || !input.starts_with("//"));
+                self.syntax_violation_if(ExpectedFileDoubleSlash, || !input.starts_with("//"));
                 let base_file_url = self.base_url.and_then(|base| {
                     if base.scheme() == "file" { Some(base) } else { None }
                 });
@@ -417,7 +409,7 @@ impl<'a> Parser<'a> {
                     }
                 }
                 // special authority slashes state
-                self.violation_fn.call_if(ExpectedDoubleSlash, || {
+                self.syntax_violation_if(ExpectedDoubleSlash, || {
                     input.clone().take_while(|&c| matches!(c, '/' | '\\'))
                     .collect::<String>() != "//"
                 });
@@ -551,10 +543,10 @@ impl<'a> Parser<'a> {
                 }
             }
             Some('/') | Some('\\') => {
-                self.violation_fn.call_if(Backslash, || first_char == Some('\\'));
+                self.syntax_violation_if(Backslash, || first_char == Some('\\'));
                 // file slash state
                 let (next_char, input_after_next_char) = input_after_first_char.split_first();
-                self.violation_fn.call_if(Backslash, || next_char == Some('\\'));
+                self.syntax_violation_if(Backslash, || next_char == Some('\\'));
                 if matches!(next_char, Some('/') | Some('\\')) {
                     // file host state
                     self.serialization.push_str("file://");
@@ -706,7 +698,7 @@ impl<'a> Parser<'a> {
             Some('/') | Some('\\') => {
                 let (slashes_count, remaining) = input.count_matching(|c| matches!(c, '/' | '\\'));
                 if slashes_count >= 2 {
-                    self.violation_fn.call_if(SyntaxViolation::ExpectedDoubleSlash, || {
+                    self.syntax_violation_if(SyntaxViolation::ExpectedDoubleSlash, || {
                         input.clone().take_while(|&c| matches!(c, '/' | '\\'))
                         .collect::<String>() != "//"
                     });
@@ -770,9 +762,9 @@ impl<'a> Parser<'a> {
             match c {
                 '@' => {
                     if last_at.is_some() {
-                        self.violation_fn.call(SyntaxViolation::UnencodedAtSign)
+                        self.syntax_violation(SyntaxViolation::UnencodedAtSign)
                     } else {
-                        self.violation_fn.call(SyntaxViolation::EmbeddedCredentials)
+                        self.syntax_violation(SyntaxViolation::EmbeddedCredentials)
                     }
                     last_at = Some((char_count, remaining.clone()))
                 },
@@ -970,7 +962,7 @@ impl<'a> Parser<'a> {
         match input.split_first() {
             (Some('/'), remaining) => input = remaining,
             (Some('\\'), remaining) => if scheme_type.is_special() {
-                self.violation_fn.call(SyntaxViolation::Backslash);
+                self.syntax_violation(SyntaxViolation::Backslash);
                 input = remaining
             },
             _ => {}
@@ -998,7 +990,7 @@ impl<'a> Parser<'a> {
                     },
                     '\\' if self.context != Context::PathSegmentSetter &&
                             scheme_type.is_special() => {
-                        self.violation_fn.call(SyntaxViolation::Backslash);
+                        self.syntax_violation(SyntaxViolation::Backslash);
                         ends_with_slash = true;
                         break
                     },
@@ -1039,7 +1031,7 @@ impl<'a> Parser<'a> {
                             self.serialization.push(':');
                         }
                         if *has_host {
-                            self.violation_fn.call(SyntaxViolation::FileWithHostAndWindowsDrive);
+                            self.syntax_violation(SyntaxViolation::FileWithHostAndWindowsDrive);
                             *has_host = false;  // FIXME account for this in callers
                         }
                     }
@@ -1181,7 +1173,7 @@ impl<'a> Parser<'a> {
     pub fn parse_fragment(&mut self, mut input: Input) {
         while let Some((c, utf8_c)) = input.next_utf8() {
             if c ==  '\0' {
-                self.violation_fn.call(SyntaxViolation::NullInFragment)
+                self.syntax_violation(SyntaxViolation::NullInFragment)
             } else {
                 self.check_url_code_point(c, &input);
                 self.serialization.extend(utf8_percent_encode(utf8_c,
@@ -1191,8 +1183,7 @@ impl<'a> Parser<'a> {
     }
 
     fn check_url_code_point(&self, c: char, input: &Input) {
-        let vfn = self.violation_fn;
-        if vfn.is_set() {
+        if let Some(vfn) = self.violation_fn {
             if c == '%' {
                 let mut input = input.clone();
                 if !matches!((input.next(), input.next()), (Some(a), Some(b))