From de1d82b4ba8cf71117bfb36ce913a8d19b172626 Mon Sep 17 00:00:00 2001
From: Benjamin Bouvier <public@benj.me>
Date: Wed, 3 Oct 2018 14:58:07 +0200
Subject: [PATCH] Make SettingsError easier to diagnose;

---
 lib/codegen/src/settings.rs | 42 ++++++++++++++++++++++++-------------
 lib/reader/src/isaspec.rs   | 13 +++++++++---
 2 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/lib/codegen/src/settings.rs b/lib/codegen/src/settings.rs
index 15274d3811..81f1ae2102 100644
--- a/lib/codegen/src/settings.rs
+++ b/lib/codegen/src/settings.rs
@@ -85,7 +85,7 @@ impl Builder {
     /// Look up a descriptor by name.
     fn lookup(&self, name: &str) -> SetResult<(usize, detail::Detail)> {
         match probe(self.template, name, simple_hash(name)) {
-            Err(_) => Err(SetError::BadName),
+            Err(_) => Err(SetError::BadName(name.to_string())),
             Ok(entry) => {
                 let d = &self.template.descriptors[self.template.hash_table[entry] as usize];
                 Ok((d.offset as usize, d.detail))
@@ -98,14 +98,17 @@ fn parse_bool_value(value: &str) -> SetResult<bool> {
     match value {
         "true" | "on" | "yes" | "1" => Ok(true),
         "false" | "off" | "no" | "0" => Ok(false),
-        _ => Err(SetError::BadValue),
+        _ => Err(SetError::BadValue("bool".to_string())),
     }
 }
 
 fn parse_enum_value(value: &str, choices: &[&str]) -> SetResult<u8> {
     match choices.iter().position(|&tag| tag == value) {
         Some(idx) => Ok(idx as u8),
-        None => Err(SetError::BadValue),
+        None => Err(SetError::BadValue(format!(
+            "any among {}",
+            choices.join(", ")
+        ))),
     }
 }
 
@@ -134,13 +137,15 @@ impl Configurable for Builder {
                 self.set_bit(offset, bit, parse_bool_value(value)?);
             }
             Detail::Num => {
-                self.bytes[offset] = value.parse().map_err(|_| SetError::BadValue)?;
+                self.bytes[offset] = value
+                    .parse()
+                    .map_err(|_| SetError::BadValue("number".to_string()))?;
             }
             Detail::Enum { last, enumerators } => {
                 self.bytes[offset] =
                     parse_enum_value(value, self.template.enums(last, enumerators))?;
             }
-            Detail::Preset => return Err(SetError::BadName),
+            Detail::Preset => return Err(SetError::BadName(name.to_string())),
         }
         Ok(())
     }
@@ -150,16 +155,16 @@ impl Configurable for Builder {
 #[derive(Fail, Debug, PartialEq, Eq)]
 pub enum SetError {
     /// No setting by this name exists.
-    #[fail(display = "No existing setting with this name")]
-    BadName,
+    #[fail(display = "No existing setting named '{}'", _0)]
+    BadName(String),
 
     /// Type mismatch for setting (e.g., setting an enum setting as a bool).
     #[fail(display = "Trying to set a setting with the wrong type")]
     BadType,
 
     /// This is not a valid value for this setting.
-    #[fail(display = "Unexpected value for a setting")]
-    BadValue,
+    #[fail(display = "Unexpected value for a setting, expected {}", _0)]
+    BadValue(String),
 }
 
 /// A result returned when changing a setting.
@@ -385,7 +390,7 @@ mod tests {
     #[test]
     fn modify_bool() {
         let mut b = builder();
-        assert_eq!(b.enable("not_there"), Err(BadName));
+        assert_eq!(b.enable("not_there"), Err(BadName("not_there".to_string())));
         assert_eq!(b.enable("enable_simd"), Ok(()));
         assert_eq!(b.set("enable_simd", "false"), Ok(()));
 
@@ -396,10 +401,19 @@ mod tests {
     #[test]
     fn modify_string() {
         let mut b = builder();
-        assert_eq!(b.set("not_there", "true"), Err(BadName));
-        assert_eq!(b.set("enable_simd", ""), Err(BadValue));
-        assert_eq!(b.set("enable_simd", "best"), Err(BadValue));
-        assert_eq!(b.set("opt_level", "true"), Err(BadValue));
+        assert_eq!(
+            b.set("not_there", "true"),
+            Err(BadName("not_there".to_string()))
+        );
+        assert_eq!(b.set("enable_simd", ""), Err(BadValue("bool".to_string())));
+        assert_eq!(
+            b.set("enable_simd", "best"),
+            Err(BadValue("bool".to_string()))
+        );
+        assert_eq!(
+            b.set("opt_level", "true"),
+            Err(BadValue("any among default, best, fastest".to_string()))
+        );
         assert_eq!(b.set("opt_level", "best"), Ok(()));
         assert_eq!(b.set("enable_simd", "0"), Ok(()));
 
diff --git a/lib/reader/src/isaspec.rs b/lib/reader/src/isaspec.rs
index 8a0e1f07b8..9132395162 100644
--- a/lib/reader/src/isaspec.rs
+++ b/lib/reader/src/isaspec.rs
@@ -43,14 +43,21 @@ where
         match opt {
             TestOption::Flag(name) => match config.enable(name) {
                 Ok(_) => {}
-                Err(SetError::BadName) => return err!(loc, "unknown flag '{}'", opt),
+                Err(SetError::BadName(name)) => return err!(loc, "unknown flag '{}'", name),
                 Err(_) => return err!(loc, "not a boolean flag: '{}'", opt),
             },
             TestOption::Value(name, value) => match config.set(name, value) {
                 Ok(_) => {}
-                Err(SetError::BadName) => return err!(loc, "unknown setting '{}'", opt),
+                Err(SetError::BadName(name)) => return err!(loc, "unknown setting '{}'", name),
                 Err(SetError::BadType) => return err!(loc, "invalid setting type: '{}'", opt),
-                Err(SetError::BadValue) => return err!(loc, "invalid setting value: '{}'", opt),
+                Err(SetError::BadValue(expected)) => {
+                    return err!(
+                        loc,
+                        "invalid setting value for '{}', expected {}",
+                        opt,
+                        expected
+                    )
+                }
             },
         }
     }