[PATCH] D151747: [llvm-jd] introduces a new tool for diffing JSON

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added a comment.

Precommit CI found issues that need to be addressed.

Thank you for working on this!



================
Comment at: llvm/docs/llvm-jd.rst:21-22
+
+Installation
+------------
+
----------------
Building instead of Installation?


================
Comment at: llvm/docs/llvm-jd.rst:60
+.. code-block::
+  :caption: Context-free grammar describing the diff language.
+
----------------
only for `-f=jd`?


================
Comment at: llvm/include/llvm/Support/JSON.h:1081-1087
+std::size_t Hash(const Array &a) noexcept;
+
+/// Computes a non-cryptographic hash for an object.
+std::size_t Hash(const Object &a) noexcept;
+
+/// Computes a non-cryptographic hash for a value.
+std::size_t Hash(const Value &v) noexcept;
----------------
Same below.

However, shouldn't we be using the functionality from https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/Hashing.h instead of rolling our own?


================
Comment at: llvm/utils/llvm-jd/llvm-jd.cpp:20-21
+#include <cstdlib>
+#include <fstream>
+#include <iostream>
+#include <optional>
----------------
I think some of this logic will be made easier with a `raw_ostream`-based interface instead of using the super slow iostream functionality from the STL, WDYT about using that? (Note, we've got `raw_fd_ostream` which has special logic for handling stdout.)


================
Comment at: llvm/utils/llvm-jd/llvm-jd.cpp:24
+#include <string>
+#include <system_error>
+#include <unordered_set>
----------------
Are we using this include?


================
Comment at: llvm/utils/llvm-jd/llvm-jd.cpp:62
+[[nodiscard]] static std::optional<std::string>
+Read(std::istream &in) noexcept {
+  in >> std::noskipws;
----------------



================
Comment at: llvm/utils/llvm-jd/llvm-jd.cpp:67-69
+  if (in.eof()) {
+    return Data;
+  }
----------------



================
Comment at: llvm/utils/llvm-jd/llvm-jd.cpp:86-88
+  if (Result) {
+    return *std::move(Result);
+  }
----------------
I'll stop commenting on style issues; you should take a pass through to make sure things match our usual style guidelines.


================
Comment at: llvm/utils/llvm-jd/llvm-jd.cpp:114
+
+// Represents the difference between two JSON values. There are three components
+// to all diffs: a path, some removals, and some additions.
----------------
I love this kind of comment, thank you for having it!


================
Comment at: llvm/utils/llvm-jd/llvm-jd.cpp:199
+                 << "' for writing, defaulting to stdout...\n";
+    goto Stdout;
+  }
----------------
Gross, but clever. :-D I think this could be made a bit more clean with a raw_fd_ostream interface so we can try to open the file and then if that fails, open stdout instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151747/new/

https://reviews.llvm.org/D151747