[PATCH] D45753: Lift JSON library from clang-tools-extra/clangd to llvm/Support.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: include/llvm/Support/JSON.h:8
+//
+//===---------------------------------------------------------------------===//
+
----------------
One thing that would help me even as I start to dig into this would be an overview comment at the top of the file...

- What is the intended / expected usage pattern?
- Mention alternatives given that it seems unlikely we'll eliminate YAMLIO immediately. Why use one vs. the other?


I'd also like to see your initial thoughts on why this library is better as a separate API/library rather than a separate interface that is part and parcel of the YAML library we already have. I don't have any real opinion (yet) about what does or doesn't make sense, and having your perspective on this would help me form an opinion I suspect.


================
Comment at: include/llvm/Support/JSON.h:26-27
+
+// An Object is a JSON object, which maps strings to heterogenous JSON values.
+// The string keys may be owned or references.
+class Object : public std::map<ObjectKey, Value> {
----------------
Feel free to defer this until higher level stuff is addressed, but here and throughout this entire file, you have excellent comments but don't use a doxygen comment prefix. I think all of these should be converted to be actual doxygen comments at whatever point this is moving forward.


Repository:
  rL LLVM

https://reviews.llvm.org/D45753