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

[Christopher Di Bella via Phabricator via llvm-commits]

cjdb added inline comments.


================
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;
----------------
aaron.ballman wrote:
> 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?
Thanks for putting this on my radar!


================
Comment at: llvm/utils/llvm-jd/llvm-jd.cpp:86-88
+  if (Result) {
+    return *std::move(Result);
+  }
----------------
aaron.ballman wrote:
> I'll stop commenting on style issues; you should take a pass through to make sure things match our usual style guidelines.
I was hoping that we could add braces since this is a new tool. Naming, on the other hand, is just negligence and shall be corrected.


================
Comment at: llvm/utils/llvm-jd/llvm-jd.cpp:429
+  return not D.empty();
+}
----------------
jroelofs wrote:
> missing trailing newline
There is a trailing newline: Phab just doesn't show it. (It'll tell you that there's a missing new line, and I have VS Code set up to ensure that there's always exactly one new line at the end of the file.)

Unless you're saying there should be two?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151747