Address review v2

CraftSpider · CraftSpider · commit 728ffc8c0391 · 2021-01-19T17:02:34.000-05:00
diff --git a/src/bootstrap/builder.rs b/src/bootstrap/builder.rs
@@ -751,11 +751,6 @@ impl<'a> Builder<'a> {
         cmd
     }
 
-    /// Gets a path to the jsondocck tool
-    pub fn jsondocck(&self, compiler: Compiler, target: TargetSelection) -> PathBuf {
-        self.ensure(tool::JsonDocCk { compiler, target })
-    }
-
     /// Return the path to `llvm-config` for the target, if it exists.
     ///
     /// Note that this returns `None` if LLVM is disabled, or if we're in a
diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs
@@ -1072,7 +1072,7 @@ note: if you're sure you want to do this, please open an issue as to why. In the
 
         cmd.arg("--docck-python").arg(builder.python());
 
-        cmd.arg("--jsondocck-path").arg(builder.jsondocck(compiler, target));
+        cmd.arg("--jsondocck-path").arg(builder.ensure(tool::JsonDocCk { compiler, target }));
 
         if builder.config.build.ends_with("apple-darwin") {
             // Force /usr/bin/python3 on macOS for LLDB tests because we're loading the
diff --git a/src/test/rustdoc-json/nested.rs b/src/test/rustdoc-json/nested.rs
@@ -1,25 +1,24 @@
 // edition:2018
 
-// @has nested.json "$.index.['0:0'].kind" \"module\"
-// @has - "$.index.['0:0'].inner.is_crate" true
-// @has - "$.index.['0:0'].inner.items[*]" \"0:3\"
+// @has nested.json "$.index[*][?(@.name=='nested')].kind" \"module\"
+// @has - "$.index[*][?(@.name=='nested')].inner.is_crate" true
+// @count - "$.index[*][?(@.name=='nested')].inner.items[*]" 1
 
-// @has nested.json "$.index.['0:3'].kind" \"module\"
-// @has - "$.index.['0:3'].inner.is_crate" false
-// @has - "$.index.['0:3'].inner.items[*]" \"0:4\"
-// @has - "$.index.['0:3'].inner.items[*]" \"0:7\"
+// @has nested.json "$.index[*][?(@.name=='l1')].kind" \"module\"
+// @has - "$.index[*][?(@.name=='l1')].inner.is_crate" false
+// @count - "$.index[*][?(@.name=='l1')].inner.items[*]" 2
 pub mod l1 {
 
-    // @has nested.json "$.index.['0:4'].kind" \"module\"
-    // @has - "$.index.['0:4'].inner.is_crate" false
-    // @has - "$.index.['0:4'].inner.items[*]" \"0:5\"
+    // @has nested.json "$.index[*][?(@.name=='l3')].kind" \"module\"
+    // @has - "$.index[*][?(@.name=='l3')].inner.is_crate" false
+    // @count - "$.index[*][?(@.name=='l3')].inner.items[*]" 1
     pub mod l3 {
 
-        // @has nested.json "$.index.['0:5'].kind" \"struct\"
-        // @has - "$.index.['0:5'].inner.struct_type" \"unit\"
+        // @has nested.json "$.index[*][?(@.name=='L4')].kind" \"struct\"
+        // @has - "$.index[*][?(@.name=='L4')].inner.struct_type" \"unit\"
         pub struct L4;
     }
-    // @has nested.json "$.index.['0:7'].kind" \"import\"
-    // @has - "$.index.['0:7'].inner.glob" false
+    // @has nested.json "$.index[*][?(@.inner.span=='l3::L4')].kind" \"import\"
+    // @has - "$.index[*][?(@.inner.span=='l3::L4')].inner.glob" false
     pub use l3::L4;
 }
diff --git a/src/test/rustdoc-json/structs/plain_empty.rs b/src/test/rustdoc-json/structs/plain_empty.rs
@@ -1,7 +1,6 @@
-// @has plain_empty.json "$.index.['0:3'].name" \"PlainEmpty\"
-// @has - "$.index.['0:3'].visibility" \"public\"
-// @has - "$.index.['0:3'].kind" \"struct\"
-// @has - "$.index.['0:3'].inner.struct_type" \"plain\"
-// @has - "$.index.['0:3'].inner.fields_stripped" false
-// @has - "$.index.['0:3'].inner.fields" []
+// @has plain_empty.json "$.index[*][?(@.name=='PlainEmpty')].visibility" \"public\"
+// @has - "$.index[*][?(@.name=='PlainEmpty')].kind" \"struct\"
+// @has - "$.index[*][?(@.name=='PlainEmpty')].inner.struct_type" \"plain\"
+// @has - "$.index[*][?(@.name=='PlainEmpty')].inner.fields_stripped" false
+// @has - "$.index[*][?(@.name=='PlainEmpty')].inner.fields" []
 pub struct PlainEmpty {}
diff --git a/src/test/rustdoc-json/structs/tuple.rs b/src/test/rustdoc-json/structs/tuple.rs
@@ -1,6 +1,5 @@
-// @has tuple.json "$.index.['0:3'].name" \"Tuple\"
-// @has - "$.index.['0:3'].visibility" \"public\"
-// @has - "$.index.['0:3'].kind" \"struct\"
-// @has - "$.index.['0:3'].inner.struct_type" \"tuple\"
-// @has - "$.index.['0:3'].inner.fields_stripped" true
+// @has tuple.json "$.index[*][?(@.name=='Tuple')].visibility" \"public\"
+// @has - "$.index[*][?(@.name=='Tuple')].kind" \"struct\"
+// @has - "$.index[*][?(@.name=='Tuple')].inner.struct_type" \"tuple\"
+// @has - "$.index[*][?(@.name=='Tuple')].inner.fields_stripped" true
 pub struct Tuple(u32, String);
diff --git a/src/test/rustdoc-json/structs/unit.rs b/src/test/rustdoc-json/structs/unit.rs
@@ -1,6 +1,5 @@
-// @has unit.json "$.index.['0:3'].name" \"Unit\"
-// @has - "$.index.['0:3'].visibility" \"public\"
-// @has - "$.index.['0:3'].kind" \"struct\"
-// @has - "$.index.['0:3'].inner.struct_type" \"unit\"
-// @has - "$.index.['0:3'].inner.fields" []
+// @has unit.json "$.index[*][?(@.name=='Unit')].visibility" \"public\"
+// @has - "$.index[*][?(@.name=='Unit')].kind" \"struct\"
+// @has - "$.index[*][?(@.name=='Unit')].inner.struct_type" \"unit\"
+// @has - "$.index[*][?(@.name=='Unit')].inner.fields" []
 pub struct Unit;
diff --git a/src/test/rustdoc-json/structs/with_generics.rs b/src/test/rustdoc-json/structs/with_generics.rs
@@ -1,14 +1,13 @@
 use std::collections::HashMap;
 
-// @has with_generics.json "$.index.['0:4'].name" \"WithGenerics\"
-// @has - "$.index.['0:4'].visibility" \"public\"
-// @has - "$.index.['0:4'].kind" \"struct\"
-// @has - "$.index.['0:4'].inner.generics.params[0].name" \"T\"
-// @has - "$.index.['0:4'].inner.generics.params[0].kind.type"
-// @has - "$.index.['0:4'].inner.generics.params[1].name" \"U\"
-// @has - "$.index.['0:4'].inner.generics.params[1].kind.type"
-// @has - "$.index.['0:4'].inner.struct_type" \"plain\"
-// @has - "$.index.['0:4'].inner.fields_stripped" true
+// @has with_generics.json "$.index[*][?(@.name=='WithGenerics')].visibility" \"public\"
+// @has - "$.index[*][?(@.name=='WithGenerics')].kind" \"struct\"
+// @has - "$.index[*][?(@.name=='WithGenerics')].inner.generics.params[0].name" \"T\"
+// @has - "$.index[*][?(@.name=='WithGenerics')].inner.generics.params[0].kind.type"
+// @has - "$.index[*][?(@.name=='WithGenerics')].inner.generics.params[1].name" \"U\"
+// @has - "$.index[*][?(@.name=='WithGenerics')].inner.generics.params[1].kind.type"
+// @has - "$.index[*][?(@.name=='WithGenerics')].inner.struct_type" \"plain\"
+// @has - "$.index[*][?(@.name=='WithGenerics')].inner.fields_stripped" true
 pub struct WithGenerics<T, U> {
     stuff: Vec<T>,
     things: HashMap<U, U>,
diff --git a/src/test/rustdoc-json/structs/with_primitives.rs b/src/test/rustdoc-json/structs/with_primitives.rs
@@ -1,10 +1,9 @@
-// @has with_primitives.json "$.index.['0:3'].name" \"WithPrimitives\"
-// @has - "$.index.['0:3'].visibility" \"public\"
-// @has - "$.index.['0:3'].kind" \"struct\"
-// @has - "$.index.['0:3'].inner.generics.params[0].name" \"\'a\"
-// @has - "$.index.['0:3'].inner.generics.params[0].kind" \"lifetime\"
-// @has - "$.index.['0:3'].inner.struct_type" \"plain\"
-// @has - "$.index.['0:3'].inner.fields_stripped" true
+// @has with_primitives.json "$.index[*][?(@.name=='WithPrimitives')].visibility" \"public\"
+// @has - "$.index[*][?(@.name=='WithPrimitives')].kind" \"struct\"
+// @has - "$.index[*][?(@.name=='WithPrimitives')].inner.generics.params[0].name" \"\'a\"
+// @has - "$.index[*][?(@.name=='WithPrimitives')].inner.generics.params[0].kind" \"lifetime\"
+// @has - "$.index[*][?(@.name=='WithPrimitives')].inner.struct_type" \"plain\"
+// @has - "$.index[*][?(@.name=='WithPrimitives')].inner.fields_stripped" true
 pub struct WithPrimitives<'a> {
     num: u32,
     s: &'a str,
diff --git a/src/tools/compiletest/src/main.rs b/src/tools/compiletest/src/main.rs
@@ -197,7 +197,6 @@ pub fn parse_config(args: Vec<String>) -> Config {
     let has_tidy = Command::new("tidy")
         .arg("--version")
         .stdout(Stdio::null())
-        .stderr(Stdio::null())
         .status()
         .map_or(false, |status| status.success());
     Config {
diff --git a/src/tools/jsondocck/src/cache.rs b/src/tools/jsondocck/src/cache.rs
@@ -13,6 +13,7 @@ pub struct Cache {
 }
 
 impl Cache {
+    /// Create a new cache, used to read files only once and otherwise store their contents.
     pub fn new(doc_dir: &str) -> Cache {
         Cache {
             root: Path::new(doc_dir).to_owned(),
@@ -32,9 +33,7 @@ impl Cache {
         }
     }
 
-    pub fn get_file(&mut self, path: &String) -> Result<String, io::Error> {
-        let path = self.resolve_path(path);
-
+    fn read_file(&mut self, path: PathBuf) -> Result<String, io::Error> {
         if let Some(f) = self.files.get(&path) {
             return Ok(f.clone());
         }
@@ -46,16 +45,22 @@ impl Cache {
         Ok(file)
     }
 
+    /// Get the text from a file. If called multiple times, the file will only be read once
+    pub fn get_file(&mut self, path: &String) -> Result<String, io::Error> {
+        let path = self.resolve_path(path);
+        self.read_file(path)
+    }
+
+    /// Parse the JSON from a file. If called multiple times, the file will only be read once.
     pub fn get_value(&mut self, path: &String) -> Result<Value, CkError> {
         let path = self.resolve_path(path);
 
         if let Some(v) = self.values.get(&path) {
             return Ok(v.clone());
         }
 
-        let file = fs::File::open(&path)?;
-
-        let val = serde_json::from_reader::<_, Value>(file)?;
+        let content = self.read_file(path.clone())?;
+        let val = serde_json::from_str::<Value>(&content)?;
 
         self.values.insert(path, val.clone());
 
diff --git a/src/tools/jsondocck/src/config.rs b/src/tools/jsondocck/src/config.rs
@@ -8,14 +8,15 @@ pub struct Config {
     pub template: String,
 }
 
+/// Create a Config from a vector of command-line arguments
 pub fn parse_config(args: Vec<String>) -> Config {
     let mut opts = Options::new();
     opts.reqopt("", "doc-dir", "Path to the documentation directory", "PATH")
         .reqopt("", "template", "Path to the template file", "PATH")
         .optflag("h", "help", "show this message");
 
     let (argv0, args_) = args.split_first().unwrap();
-    if args.len() == 1 || args[1] == "-h" || args[1] == "--help" {
+    if args.len() == 1 {
         let message = format!("Usage: {} <doc-dir> <template>", argv0);
         println!("{}", opts.usage(&message));
         std::process::exit(1);
diff --git a/src/tools/jsondocck/src/main.rs b/src/tools/jsondocck/src/main.rs
@@ -106,6 +106,8 @@ fn print_err(msg: &str, lineno: usize) {
     eprintln!("Invalid command: {} on line {}", msg, lineno)
 }
 
+/// Get a list of commands from a file. Does the work of ensuring the commands
+/// are syntactically valid.
 fn get_commands(template: &str) -> Result<Vec<Command>, ()> {
     let mut commands = Vec::new();
     let mut errors = false;
@@ -147,10 +149,7 @@ fn get_commands(template: &str) -> Result<Vec<Command>, ()> {
             }
         }
 
-        let args = match cap.name("args") {
-            Some(m) => shlex::split(m.as_str()).unwrap(),
-            None => vec![],
-        };
+        let args = cap.name("args").map_or(vec![], |m| shlex::split(m.as_str()).unwrap());
 
         if !cmd.validate(&args, commands.len(), lineno) {
             errors = true;
@@ -163,15 +162,14 @@ fn get_commands(template: &str) -> Result<Vec<Command>, ()> {
     if !errors { Ok(commands) } else { Err(()) }
 }
 
+/// Performs the actual work of ensuring a command passes. Generally assumes the command
+/// is syntactically valid.
 fn check_command(command: Command, cache: &mut Cache) -> Result<(), CkError> {
     let result = match command.kind {
         CommandKind::Has => {
             match command.args.len() {
                 // @has <path> = file existence
-                1 => match cache.get_file(&command.args[0]) {
-                    Ok(_) => true,
-                    Err(_) => false,
-                },
+                1 => cache.get_file(&command.args[0]).is_ok(),
                 // @has <path> <jsonpath> = check path exists
                 2 => {
                     let val = cache.get_value(&command.args[0])?;
